Add builder for EngineConfig by sachinpkale · Pull Request #5618 · opensearch-project/OpenSearch

sachinpkale · 2022-12-22T16:33:26Z

Signed-off-by: Sachin Kale kalsac@amazon.com

Description

EngineConfig class has 25+ fields and 3 constructors. Adding any new optional field results in addition of one or more constructors to keep the backward compatibility.
To avoid this, we introduced Builder class for EngineConfig.

Issues Resolved

NA

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2022-12-22T17:01:03Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/8352/
CommitID: 6da9391
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2022-12-22T17:08:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.index.IndexServiceTests.testAsyncTranslogTrimTaskOnClosedIndex

URL: https://build.ci.opensearch.org/job/gradle-check/8353/
CommitID: a9e51b0
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

andrross

Thanks for doing this! Just a few nitpicks.

andrross · 2022-12-22T17:08:09Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+            return this;
+        }
+
+        public EngineConfig createEngineConfig() {


Nitpick, but create would be fine here since "engine config" is in the class name

andrross · 2022-12-22T17:10:13Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+        }
+
+        public EngineConfig createEngineConfig() {
+            return new EngineConfig(


Another nitpick...assuming this constructor can be made private, then you can pass the this instance of EngineConfig.Builder and let the constructor read the fields. It's a bit more concise and makes adding a new field require fewer changes.

I like this. Will change in next commit.

+1 for the review comment from @andrross. Without a private constructor here, the builder for EngineConfig can be bypassed.

andrross · 2022-12-22T17:11:24Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+        private Supplier<RetentionLeases> retentionLeasesSupplier;
+        private LongSupplier primaryTermSupplier;
+        private TombstoneDocSupplier tombstoneDocSupplier;
+        private TranslogDeletionPolicyFactory translogDeletionPolicyFactory = null;


All the others fields are implicitly assigned to null. Can probably remove this for consistency.

dreamer-89

Thanks @sachinpkale for this change. This improves the readability and removes lot of unnecessary code.

dreamer-89 · 2022-12-22T17:25:46Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+        private LongSupplier primaryTermSupplier;
+        private TombstoneDocSupplier tombstoneDocSupplier;
+        private TranslogDeletionPolicyFactory translogDeletionPolicyFactory = null;
+        private boolean isReadOnlyReplica = false;


isReadOnlyReplica will have false as default, so it can be removed as well.

dreamer-89 · 2022-12-22T17:32:27Z

server/src/test/java/org/opensearch/index/engine/EngineConfigTests.java

+        EngineConfig config = new EngineConfig.Builder().setShardId(null)
+            .setThreadPool(null)
+            .setIndexSettings(defaultIndexSettings)
+            .setWarmer(null)
+            .setStore(null)
+            .setMergePolicy(null)
+            .setAnalyzer(null)
+            .setSimilarity(null)
+            .setCodecService(null)
+            .setEventListener(null)
+            .setQueryCache(null)
+            .setQueryCachingPolicy(null)
+            .setTranslogConfig(null)
+            .setFlushMergesAfter(null)
+            .setExternalRefreshListener(null)
+            .setInternalRefreshListener(null)
+            .setIndexSort(null)
+            .setCircuitBreakerService(null)
+            .setGlobalCheckpointSupplier(null)
+            .setRetentionLeasesSupplier(() -> RetentionLeases.EMPTY)
+            .setPrimaryTermSupplier(null)
+            .setTombstoneDocSupplier(null)


We dont' need to use setXXX for settings null values as it is default. This is one of the advantages of using builder pattern as it replaces large constructor with default values.

Thanks @dreamer-89 . I used IntelliJ's Replace constructor with builder and did not pay much attention to these optimizations. Will take care of these in next commit.

dreamer-89 · 2022-12-22T17:32:38Z

server/src/test/java/org/opensearch/index/engine/EngineConfigTests.java

+        return new EngineConfig.Builder().setShardId(null)
+            .setThreadPool(null)
+            .setIndexSettings(indexSettings)
+            .setWarmer(null)
+            .setStore(null)
+            .setMergePolicy(null)
+            .setAnalyzer(null)
+            .setSimilarity(null)
+            .setCodecService(null)
+            .setEventListener(null)
+            .setQueryCache(null)
+            .setQueryCachingPolicy(null)
+            .setTranslogConfig(null)
+            .setFlushMergesAfter(null)
+            .setExternalRefreshListener(null)
+            .setInternalRefreshListener(null)
+            .setIndexSort(null)
+            .setCircuitBreakerService(null)
+            .setGlobalCheckpointSupplier(null)
+            .setRetentionLeasesSupplier(() -> RetentionLeases.EMPTY)
+            .setPrimaryTermSupplier(null)
+            .setTombstoneDocSupplier(null)
+            .setIsReadOnlyReplica(true)
+            .createEngineConfig();


Same as above.

dreamer-89 · 2022-12-22T17:33:35Z

server/src/test/java/org/opensearch/index/shard/RefreshListenersTests.java

+        EngineConfig config = new EngineConfig.Builder().setShardId(shardId)
+            .setThreadPool(threadPool)
+            .setIndexSettings(indexSettings)
+            .setWarmer(null)


This and other fields set to null can be omitted as mentioned above.

Signed-off-by: Sachin Kale <kalsac@amazon.com>

andrross

Suggested another refactoring, but I'll leave it up to you whether to do it here or in a follow up.

andrross · 2022-12-22T18:36:02Z

server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java

-            config.getPrimaryTermSupplier(),
-            config.getTombstoneDocSupplier()
-        );
+        EngineConfig configWithCustomTranslogDeletionPolicyFactory = new EngineConfig.Builder().setShardId(config.getShardId())


There are multiple cases of taking an existing config and making a small change to it in these tests. What do you think about adding a toBuilder() method that would make this more concise and more clear. This case would look like:

config.toBuilder().setTranslogDeletionPolicyFactory(translogDeletionPolicyFactory).create();

Agree, but changing all these tests to use toBuilder would take some time. I would like to do it as a follow-up.

I am also thinking of using Lombok in OpenSearch which will get rid of such boilerplate code (in other classes as well). I will create a tracking issue for the same.

github-actions · 2022-12-22T18:53:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.client.ReindexIT.testReindexTask

URL: https://build.ci.opensearch.org/job/gradle-check/8362/
CommitID: 10fcab9
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

BhumikaSaini · 2022-12-22T19:03:44Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+            return this;
+        }
+
+        public Builder setTranslogFactory(TranslogFactory translogFactory) {


Nitpick:

For the setters in a builder we usually drop the set prefix in favour of a fluent design

i.e. public Builder translogFactory(TranslogFactory translogFactory) over public Builder setTranslogFactory(TranslogFactory translogFactory)

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2022-12-23T04:22:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/8379/
CommitID: 7f77ca6
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2022-12-23T04:24:07Z

Codecov Report

Merging #5618 (7f77ca6) into main (306b199) will increase coverage by 0.03%.
The diff coverage is 89.68%.

@@             Coverage Diff              @@
##               main    #5618      +/-   ##
============================================
+ Coverage     70.84%   70.88%   +0.03%     
+ Complexity    58296    58294       -2     
============================================
  Files          4741     4741              
  Lines        278541   278642     +101     
  Branches      40268    40268              
============================================
+ Hits         197322   197503     +181     
+ Misses        65068    65025      -43     
+ Partials      16151    16114      -37

Impacted Files	Coverage Δ
...va/org/opensearch/index/engine/EngineTestCase.java	`86.07% <80.00%> (-0.35%)`	⬇️
...java/org/opensearch/index/engine/EngineConfig.java	`98.41% <100.00%> (+1.01%)`	⬆️
...g/opensearch/index/engine/EngineConfigFactory.java	`92.75% <100.00%> (+4.11%)`	⬆️
...n/indices/forcemerge/ForceMergeRequestBuilder.java	`0.00% <0.00%> (-75.00%)`	⬇️
.../indices/forcemerge/TransportForceMergeAction.java	`25.00% <0.00%> (-75.00%)`	⬇️
...h/action/ingest/SimulateDocumentVerboseResult.java	`60.71% <0.00%> (-39.29%)`	⬇️
...arch/search/aggregations/pipeline/LinearModel.java	`23.07% <0.00%> (-34.62%)`	⬇️
...on/admin/indices/forcemerge/ForceMergeRequest.java	`50.00% <0.00%> (-31.58%)`	⬇️
...rc/main/java/org/opensearch/ingest/IngestInfo.java	`51.72% <0.00%> (-27.59%)`	⬇️
...a/org/opensearch/index/mapper/MapperException.java	`75.00% <0.00%> (-25.00%)`	⬇️
... and 481 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Bukhtawar · 2022-12-23T04:46:36Z

server/src/main/java/org/opensearch/index/engine/EngineConfig.java

+     *
+     * @opensearch.internal
+     */
+    public static class Builder {


Will this Builder make certain ctor parameters optional? How do we know we don't miss out on all mandatory ones

The build method still calls the constructor (which is private now) so all the validations on parameters remain same.

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)

opensearch-trigger-bot · 2022-12-27T18:36:10Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5618-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4fe809784c356d3767d8baddf35d0612e747a0d0
# Push it to GitHub
git push --set-upstream origin backport/backport-5618-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-5618-to-2.x.

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com> Signed-off-by: Ashish Singh <ssashish@amazon.com>

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com>

Sachin Kale added 2 commits December 22, 2022 21:58

Add builder for EngineConfig

6da9391

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Add javadoc for EngineConfig.Builder class

a9e51b0

Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale marked this pull request as ready for review December 22, 2022 16:39

sachinpkale requested review from Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, adnapibar, anasalkouz, andrross, dblock, dreamer-89, kartg, kotwanikunal, mch2, nknize, owaiskazi19, reta, ryanbogan, saratvemulapalli, setiah, shwetathareja, tlfeng and xuezhou25 as code owners December 22, 2022 16:39

sachinpkale added the skip-changelog label Dec 22, 2022

andrross reviewed Dec 22, 2022

View reviewed changes

dreamer-89 reviewed Dec 22, 2022

View reviewed changes

Address PR comments

10fcab9

Signed-off-by: Sachin Kale <kalsac@amazon.com>

andrross approved these changes Dec 22, 2022

View reviewed changes

BhumikaSaini reviewed Dec 22, 2022

View reviewed changes

Remove set prefix from builder's setter methods

7f77ca6

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Bukhtawar reviewed Dec 23, 2022

View reviewed changes

BhumikaSaini approved these changes Dec 23, 2022

View reviewed changes

Bukhtawar approved these changes Dec 23, 2022

View reviewed changes

Bukhtawar merged commit 4fe8097 into opensearch-project:main Dec 23, 2022

andrross added the backport 2.x Backport to 2.x branch label Dec 27, 2022

sachinpkale mentioned this pull request Jan 9, 2023

[Backport 2.x] Add support for refresh level durability using remote segment store #5749

Merged

6 tasks

ashking94 mentioned this pull request Jan 18, 2023

Handle translog upload during primary relocation for remote-backed indexes #5804

Merged

6 tasks

ashking94 pushed a commit to ashking94/OpenSearch that referenced this pull request Jan 18, 2023

Add builder for EngineConfig (opensearch-project#5618)

dd21fdb

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com> Signed-off-by: Ashish Singh <ssashish@amazon.com>

ashking94 mentioned this pull request Jan 18, 2023

[Backport 2.x] Add builder for EngineConfig (#5618) #5914

Merged

6 tasks

ashking94 pushed a commit to ashking94/OpenSearch that referenced this pull request Jan 18, 2023

Add builder for EngineConfig (opensearch-project#5618)

34d6294

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com> Signed-off-by: Ashish Singh <ssashish@amazon.com>

ashking94 pushed a commit to ashking94/OpenSearch that referenced this pull request Jan 18, 2023

Add builder for EngineConfig (opensearch-project#5618)

415cdef

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com> Signed-off-by: Ashish Singh <ssashish@amazon.com>

gbbafna pushed a commit that referenced this pull request Jan 18, 2023

Add builder for EngineConfig (#5618) (#5914)

b4e01bf

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com>

kotwanikunal pushed a commit that referenced this pull request Jan 25, 2023

Add builder for EngineConfig (#5618) (#5914)

0a8efea

* Add builder for EngineConfig Signed-off-by: Sachin Kale <kalsac@amazon.com>

bharath-techie mentioned this pull request Jun 25, 2024

[Feature Request] [Gradle Checks] Allow annotation of InternalApi on classes not meant to be consumed outside of OpenSearch #13308

Closed

Conversation

sachinpkale commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Dec 22, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Dec 22, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

andrross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamer-89 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrross left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 22, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 23, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Dec 23, 2022

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Dec 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sachinpkale commented Dec 22, 2022 •

edited

Loading