Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace TokenizerFactory with Supplier #32063

Merged
merged 4 commits into from
Jul 17, 2018
Merged

Replace TokenizerFactory with Supplier #32063

merged 4 commits into from
Jul 17, 2018

Conversation

original-brownbear
Copy link
Member

Handles TODOs from #24869

  • Replaces all occurences of TokenizerFactory with Supplier<Tokenizer>
  • Remove unused parameter from constructor

@original-brownbear
Replace TokenizerFactory with Supplier
5b86b7f 
Handles TODOs from elastic#24869
* Replaces all occurences of TokenizerFactory with Supplier<Tokenizer>
* Remove unused parameter from constructor
@original-brownbear original-brownbear added the :Core/Infra/Plugins Plugin API and infrastructure label Jul 15, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jpountz
Copy link
Contributor

jpountz commented Jul 15, 2018

I'm not entirely convinced by this change since it removes existing consistency with the char filter and token filter factories?

@original-brownbear
Copy link
Member Author

@jpountz I guess with @nik9000 removing the name() methods from those factories those could be turned into Supplier<?>s as well? (just guessing here though, I'm fine closing this if it's just noise :))

@jpountz
Copy link
Contributor

jpountz commented Jul 16, 2018

I see. I guess I'm still a bit torn since this is part of our plugin API so we could regret replacing these factories with a supplier which doesn't allow adding new methods with a default implementation. So this might prevent from introducing new functionality in minor releases. @nik9000 what do you think?

@nik9000
Copy link
Member

nik9000 commented Jul 16, 2018

We're still a ways off from semvering the plugin API so I think it'd be fine. Though it looks like I should revive my old PR about removing the names....

nik9000
nik9000 approved these changes Jul 16, 2018
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the change if @jpountz is on board with it.

@jpountz
Copy link
Contributor

jpountz commented Jul 16, 2018

Even if the plugin API is not officially stable, this change is breaking, doesn't provide much value and could prevent from introducing changes in a backward-compatible way in the future so I'd rather not merge it. Sorry @original-brownbear I hope you didn't spend too much time on this one.

@original-brownbear
Copy link
Member Author

@jpountz no worries I didn't :)

... that said, should I maybe just revert all the Supplierish changes in here, keep the name removal and just remove the Supplier todo?

@jpountz
Copy link
Contributor

jpountz commented Jul 16, 2018

+1

@original-brownbear
Copy link
Member Author

done :)

@original-brownbear
Copy link
Member Author

@jpountz can you take another look when you have a minute? <= Should be a really short one now :)

jpountz
jpountz approved these changes Jul 17, 2018
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I had written a LGTM yesterday but I must have forgotten to hit the "submit" button. Thank you.

@original-brownbear
Copy link
Member Author

@jpountz np+thanks! :) label+merge to 7.0 and 6.x right?

@original-brownbear original-brownbear merged commit ed3b44f into elastic:master Jul 17, 2018
@original-brownbear original-brownbear deleted the handle-tokenizer-todos branch July 17, 2018 12:14
@jpountz
Copy link
Contributor

jpountz commented Jul 17, 2018

I'd only do 7.0 to not break existing analysis plugins.

dnhatn added a commit that referenced this pull request Jul 20, 2018
@dnhatn
Merge branch 'master' into ccr
d0f3ed5 
* master:
  Painless: Simplify Naming in Lookup Package (#32177)
  Handle missing values in painless (#32207)
  add support for write index resolution when creating/updating documents (#31520)
  ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864)
  Remove indication of future multi-homing support (#32187)
  Rest test - allow for snapshots to take 0 milliseconds
  Make x-pack-core generate a pom file
  Rest HL client: Add put watch action (#32026)
  Build: Remove pom generation for plugin zip files (#32180)
  Fix comments causing errors with Java 11
  Fix rollup on date fields that don't support epoch_millis (#31890)
  Detect and prevent configuration that triggers a Gradle bug (#31912)
  [test] port linux package packaging tests (#31943)
  Revert "Introduce a Hashing Processor (#31087)" (#32178)
  Remove empty @return from JavaDoc
  Adjust SSLDriver behavior for JDK11 changes (#32145)
  [test] use randomized runner in packaging tests (#32109)
  Add support for field aliases. (#32172)
  Painless: Fix caching bug and clean up addPainlessClass. (#32142)
  Call setReferences() on custom referring tokenfilters in _analyze (#32157)
  Fix BwC Tests looking for UUID Pre 6.4 (#32158)
  Improve docs for search preferences (#32159)
  use before instead of onOrBefore
  Add more contexts to painless execute api (#30511)
  Add EC2 credential test for repository-s3 (#31918)
  A replica can be promoted and started in one cluster state update (#32042)
  Fix Java 11 javadoc compile problem
  Fix CP for namingConventions when gradle home has spaces (#31914)
  Fix `range` queries on `_type` field for singe type indices (#31756)
  [DOCS] Update TLS on Docker for 6.3 (#32114)
  ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are
  Remove versionType from translog (#31945)
  Switch distribution to new style Requests (#30595)
  Build: Skip jar tests if jar disabled
  Painless: Add PainlessClassBuilder (#32141)
  Build: Make additional test deps of check (#32015)
  Disable C2 from using AVX-512 on JDK 10 (#32138)
  Build: Move shadow customizations into common code (#32014)
  Painless: Fix Bug with Duplicate PainlessClasses (#32110)
  Remove empty @param from Javadoc
  Re-disable packaging tests on suse boxes
  Docs: Fix missing example script quote (#32010)
  [ML] Wait for aliases in multi-node tests (#32086)
  [ML] Move analyzer dependencies out of categorization config (#32123)
  Ensure to release translog snapshot in primary-replica resync (#32045)
  Handle TokenizerFactory  TODOs (#32063)
  Relax TermVectors API to work with textual fields other than TextFieldType (#31915)
  Updates the build to gradle 4.9 (#32087)
  Mute :qa:mixed-cluster indices.stats/10_index/Index - all’
  Check that client methods match API defined in the REST spec (#31825)
  Enable testing in FIPS140 JVM (#31666)
  Fix put mappings java API documentation (#31955)
  Add exclusion option to `keep_types` token filter (#32012)
  [Test] Modify assert statement for ssl handshake (#32072)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Plugins Plugin API and infrastructure >non-issue v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@original-brownbear @elasticmachine @jpountz @nik9000 @colings86