Add builder for EngineConfig #5618
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -154,114 +154,6 @@ public Supplier<RetentionLeases> retentionLeasesSupplier() { | |
|
|
||
| private final TranslogFactory translogFactory; | ||
|
|
||
| /** | ||
| * Creates a new {@link org.opensearch.index.engine.EngineConfig} | ||
| */ | ||
|
|
@@ -573,4 +465,192 @@ public TombstoneDocSupplier getTombstoneDocSupplier() { | |
| public TranslogDeletionPolicyFactory getCustomTranslogDeletionPolicyFactory() { | ||
| return translogDeletionPolicyFactory; | ||
| } | ||
|
|
||
| /** | ||
| * Builder for EngineConfig class | ||
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public static class Builder { | ||
| private ShardId shardId; | ||
| private ThreadPool threadPool; | ||
| private IndexSettings indexSettings; | ||
| private Engine.Warmer warmer; | ||
| private Store store; | ||
| private MergePolicy mergePolicy; | ||
| private Analyzer analyzer; | ||
| private Similarity similarity; | ||
| private CodecService codecService; | ||
| private Engine.EventListener eventListener; | ||
| private QueryCache queryCache; | ||
| private QueryCachingPolicy queryCachingPolicy; | ||
| private TranslogConfig translogConfig; | ||
| private TimeValue flushMergesAfter; | ||
| private List<ReferenceManager.RefreshListener> externalRefreshListener; | ||
| private List<ReferenceManager.RefreshListener> internalRefreshListener; | ||
| private Sort indexSort; | ||
| private CircuitBreakerService circuitBreakerService; | ||
| private LongSupplier globalCheckpointSupplier; | ||
| private Supplier<RetentionLeases> retentionLeasesSupplier; | ||
| private LongSupplier primaryTermSupplier; | ||
| private TombstoneDocSupplier tombstoneDocSupplier; | ||
| private TranslogDeletionPolicyFactory translogDeletionPolicyFactory = null; | ||
|
There was a problem hiding this comment. All the others fields are implicitly assigned to null. Can probably remove this for consistency. |
||
| private boolean isReadOnlyReplica = false; | ||
|
There was a problem hiding this comment.
|
||
| private TranslogFactory translogFactory = new InternalTranslogFactory(); | ||
|
|
||
| public Builder setShardId(ShardId shardId) { | ||
| this.shardId = shardId; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setThreadPool(ThreadPool threadPool) { | ||
| this.threadPool = threadPool; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setIndexSettings(IndexSettings indexSettings) { | ||
| this.indexSettings = indexSettings; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setWarmer(Engine.Warmer warmer) { | ||
| this.warmer = warmer; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setStore(Store store) { | ||
| this.store = store; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setMergePolicy(MergePolicy mergePolicy) { | ||
| this.mergePolicy = mergePolicy; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setAnalyzer(Analyzer analyzer) { | ||
| this.analyzer = analyzer; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setSimilarity(Similarity similarity) { | ||
| this.similarity = similarity; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setCodecService(CodecService codecService) { | ||
| this.codecService = codecService; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setEventListener(Engine.EventListener eventListener) { | ||
| this.eventListener = eventListener; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setQueryCache(QueryCache queryCache) { | ||
| this.queryCache = queryCache; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setQueryCachingPolicy(QueryCachingPolicy queryCachingPolicy) { | ||
| this.queryCachingPolicy = queryCachingPolicy; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setTranslogConfig(TranslogConfig translogConfig) { | ||
| this.translogConfig = translogConfig; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setFlushMergesAfter(TimeValue flushMergesAfter) { | ||
| this.flushMergesAfter = flushMergesAfter; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setExternalRefreshListener(List<ReferenceManager.RefreshListener> externalRefreshListener) { | ||
| this.externalRefreshListener = externalRefreshListener; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setInternalRefreshListener(List<ReferenceManager.RefreshListener> internalRefreshListener) { | ||
| this.internalRefreshListener = internalRefreshListener; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setIndexSort(Sort indexSort) { | ||
| this.indexSort = indexSort; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setCircuitBreakerService(CircuitBreakerService circuitBreakerService) { | ||
| this.circuitBreakerService = circuitBreakerService; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setGlobalCheckpointSupplier(LongSupplier globalCheckpointSupplier) { | ||
| this.globalCheckpointSupplier = globalCheckpointSupplier; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setRetentionLeasesSupplier(Supplier<RetentionLeases> retentionLeasesSupplier) { | ||
| this.retentionLeasesSupplier = retentionLeasesSupplier; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setPrimaryTermSupplier(LongSupplier primaryTermSupplier) { | ||
| this.primaryTermSupplier = primaryTermSupplier; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setTombstoneDocSupplier(TombstoneDocSupplier tombstoneDocSupplier) { | ||
| this.tombstoneDocSupplier = tombstoneDocSupplier; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setTranslogDeletionPolicyFactory(TranslogDeletionPolicyFactory translogDeletionPolicyFactory) { | ||
| this.translogDeletionPolicyFactory = translogDeletionPolicyFactory; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setIsReadOnlyReplica(boolean isReadOnlyReplica) { | ||
| this.isReadOnlyReplica = isReadOnlyReplica; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder setTranslogFactory(TranslogFactory translogFactory) { | ||
|
There was a problem hiding this comment. Nitpick: For the setters in a builder we usually drop the i.e. |
||
| this.translogFactory = translogFactory; | ||
| return this; | ||
| } | ||
|
|
||
| public EngineConfig createEngineConfig() { | ||
|
There was a problem hiding this comment. Nitpick, but |
||
| return new EngineConfig( | ||
|
There was a problem hiding this comment. Another nitpick...assuming this constructor can be made private, then you can pass the There was a problem hiding this comment. I like this. Will change in next commit. There was a problem hiding this comment. +1 for the review comment from @andrross. Without a private constructor here, the builder for |
||
| shardId, | ||
| threadPool, | ||
| indexSettings, | ||
| warmer, | ||
| store, | ||
| mergePolicy, | ||
| analyzer, | ||
| similarity, | ||
| codecService, | ||
| eventListener, | ||
| queryCache, | ||
| queryCachingPolicy, | ||
| translogConfig, | ||
| translogDeletionPolicyFactory, | ||
| flushMergesAfter, | ||
| externalRefreshListener, | ||
| internalRefreshListener, | ||
| indexSort, | ||
| circuitBreakerService, | ||
| globalCheckpointSupplier, | ||
| retentionLeasesSupplier, | ||
| primaryTermSupplier, | ||
| tombstoneDocSupplier, | ||
| isReadOnlyReplica, | ||
| translogFactory | ||
| ); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this Builder make certain ctor parameters optional? How do we know we don't miss out on all mandatory ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
buildmethod still calls the constructor (which is private now) so all the validations on parameters remain same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on the validation but the notion of what are mandatory parameters and what are optional is diluted. Previously everything was mandatory, but is there something we could do to ease that and make this of the type EngineConfig#builder(mandatory_params).optionalParam(param).build(this)