Enable engine factory to be pluggable #26827
Conversation
This commit enables the engine factory to be pluggable based on index settings used when creating the index service for an index.
jasontedor
force-pushed
the
engine-factory-provider
branch
from
497a8ad to
ce7e52e
Compare
| * | ||
| * @return the engine factory provider | ||
| */ | ||
| EngineFactoryProvider getEngineFactoryProvider(); |
There was a problem hiding this comment.
do we really need the indirection here and can't we simply have a single factory method:
public EngineFactory getEngineFactory(IndexSettings settings);I don't understand why we have to have another indirection?
This commit removes a level of indirection by removing the engine factory provider abstraction.
* xdcr: Maybe die before trying to log cause Log cause when a write and flush fails Die if write listener fails due to fatal error RecoveryIT.testHistoryUUIDIsGenerated should reduce unassigned shards delay instead of ensure green. Replace group map settings with affix setting (elastic#26819) Fix references to vm.max_map_count in Docker docs Add more instructions about jar hell (elastic#26837) Forbid negative values for index.unassigned.node_left.delayed_timeout (elastic#26828) Nitpicking typos in comments (elastic#26831) MetaData Builder doesn't properly prevent an alias with the same name as an index (elastic#26804)
jasontedor
force-pushed
the
engine-factory-provider
branch
from
0d2bbf9 to
cdf80a8
Compare
Since EnginePlugin is an interface, a final modifier on a parameter in a method declared there is redundant.
| * | ||
| * @return an optional engine factory | ||
| */ | ||
| Optional<EngineFactory> getMaybeEngineFactory(IndexSettings indexSettings); |
There was a problem hiding this comment.
Why the use of Optional? In other places we have just used null to indicate there is no engine implementation being added. Also, can I don't think we should use the Maybe style of naming. Again, in the pull based plugin APIs we have not done this anywhere else, and I don't think it really adds anything.
There was a problem hiding this comment.
I agree on renaming getMaybeEngineFactory() to getEngineFactory() the fact that it returns an optional indicates that nothing maybe returned. Personally I'm both fine with returning an Optional or just null.
There was a problem hiding this comment.
+1 to null and rename to getEngineFactory
There was a problem hiding this comment.
I prefer the use of optional, but I'm fine with renaming.
There was a problem hiding this comment.
I think we should come to an agreement on whether we should use Optional for these, and be consistent. But arbitrarily adding inconsistencies into the API will just create confusion.
There was a problem hiding this comment.
I agree on the consistency argument. We discussed Optional versus null in a separate channel that we will merge this as-is for now (with Optional), consider changing the handful of other places in the plugin API where we allow null to be returned but will have a broader discussion about this first. We can return to this change if needed.
| return "[" + t.v1().getClass().getName() + "/" + t.v2().get().getClass().getName() + "]"; | ||
| }) | ||
| .collect(Collectors.joining(","))); | ||
| throw new IllegalStateException(message); |
There was a problem hiding this comment.
Just an idea and not sure how feasible it is, but besides failing here, it would also be great if we can fail more directly in the create index api? (for example in MetaDataCreateIndexService#validateIndexSettings(...) method). This way the error is directly visible and an index with illegal settings will not be created.
| * | ||
| * @return an optional engine factory | ||
| */ | ||
| Optional<EngineFactory> getMaybeEngineFactory(IndexSettings indexSettings); |
There was a problem hiding this comment.
I agree on renaming getMaybeEngineFactory() to getEngineFactory() the fact that it returns an optional indicates that nothing maybe returned. Personally I'm both fine with returning an Optional or just null.
* ccr: (42 commits) [DOCS] Added info about snapshotting your data before an upgrade. Add documentation about disabling `_field_names`. (elastic#26813) Remove UnsortedNumericDoubleValues (elastic#26817) Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856) [TEST] Added skipping the `headers` feature to the Bulk REST YAML test Update type-field.asciidoc Fix search_after with geo distance sorting (elastic#26891) Use proper logging placeholder for Netty logging Add Netty channel information on write and flush failure Remove deploying in JBoss documentation Document JVM option MaxFDLimit for macOS () Add additional low-level logging handler () Unwrap causes when maybe dying Change log level on write and flush failure to warn [TEST] add test to ensure legacy list syntax in yml works fine Bump BWC version for settings serialization to 6.1.0 Removed void token filter entries and added two tests Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238) Fix toString() in SnapshotStatus (elastic#26852) elastic#26870 change bwc version for fuzzy_transpositions to 6.1 after backport ...
* ccr: Use LF line endings in Painless generated files (elastic#26822)
|
LGTM too. We can discuss Optional vs null next week. |
This commit enables the engine factory to be pluggable based on index settings used when creating the index service for an index. Relates #26827
clintongormley
added
:Distributed Indexing/Distributed
clintongormley
removed
the
:Distributed Indexing/Distributed
This commit enables the engine factory to be pluggable based on index settings used when creating the index service for an index.