Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config. #7516

jtao15 · 2021-10-04T22:59:01Z

Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config.

jackjlli · 2021-10-04T23:19:41Z

pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java

@@ -102,7 +102,12 @@ public void init(PinotConfiguration config)
    setupHelixSystemProperties();
    _helixManager = new ZKHelixManager(helixClusterName, _instanceId, InstanceType.PARTICIPANT, zkAddress);
    MinionTaskZkMetadataManager minionTaskZkMetadataManager = new MinionTaskZkMetadataManager(_helixManager);
-    _taskExecutorFactoryRegistry = new TaskExecutorFactoryRegistry(minionTaskZkMetadataManager);
+    int endReplaceSegmentsSocketTimeoutMs =
+        _config.getProperty(CommonConstants.Minion.END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS_KEY) != null


Is there any existing method like getOrDefault that you can use to make it clean?

Good point, using the getOrDefault function instead.

jackjlli · 2021-10-04T23:22:24Z

pinot-minion/src/main/java/org/apache/pinot/minion/executor/PinotTaskExecutorFactory.java

@@ -26,7 +26,7 @@
  /**
   * Initializes the task executor factory.
   */
-  void init(MinionTaskZkMetadataManager zkMetadataManager);
+  void init(MinionTaskZkMetadataManager zkMetadataManager, int endReplaceSegmentsSocketTimeoutMs);


not a big fan of changing the interface here as this interface is not just used by merge and roll up task. Is it possible to put it inside the zkMetadataManager?

Updated to store the value in zkMetadataManager

codecov-commenter · 2021-10-04T23:31:43Z

Codecov Report

Merging #7516 (aaeeb58) into master (f306460) will decrease coverage by 41.95%.
The diff coverage is 100.00%.

❗ Current head aaeeb58 differs from pull request most recent head 29c3d61. Consider uploading reports for the commit 29c3d61 to get more accurate results

@@              Coverage Diff              @@
##             master    #7516       +/-   ##
=============================================
- Coverage     72.60%   30.65%   -41.96%     
=============================================
  Files          1523     1514        -9     
  Lines         75609    75264      -345     
  Branches      11022    10983       -39     
=============================================
- Hits          54895    23069    -31826     
- Misses        17018    50143    +33125     
+ Partials       3696     2052     -1644

Flag	Coverage Δ
integration1	`30.65% <100.00%> (-0.07%)`	⬇️
integration2	`?`
unittests1	`?`
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../org/apache/pinot/core/common/MinionConstants.java	`0.00% <ø> (ø)`
...on/tasks/mergerollup/MergeRollupTaskGenerator.java	`75.27% <ø> (-12.46%)`	⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java	`0.00% <ø> (-26.32%)`	⬇️
...ava/org/apache/pinot/minion/BaseMinionStarter.java	`75.43% <100.00%> (-2.24%)`	⬇️
...t/minion/executor/MinionTaskZkMetadataManager.java	`100.00% <100.00%> (ø)`
.../tasks/BaseMultipleSegmentsConversionExecutor.java	`87.91% <100.00%> (+1.09%)`	⬆️
...ion/tasks/mergerollup/MergeRollupTaskExecutor.java	`100.00% <100.00%> (ø)`
...ks/mergerollup/MergeRollupTaskExecutorFactory.java	`100.00% <100.00%> (ø)`
...egments/RealtimeToOfflineSegmentsTaskExecutor.java	`92.53% <100.00%> (-1.59%)`	⬇️
...c/main/java/org/apache/pinot/common/tier/Tier.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1063 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f306460...29c3d61. Read the comment docs.

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

snleee · 2021-10-05T00:53:17Z

pinot-minion/src/main/java/org/apache/pinot/minion/executor/MinionTaskZkMetadataManager.java


-  public MinionTaskZkMetadataManager(HelixManager helixManager) {
+  public MinionTaskZkMetadataManager(HelixManager helixManager, int endReplaceSegmentsSocketTimeoutMs) {


+1 that we should not change the interface. However, adding this config to MinionTaskZkMetadataManager looks like a hacky solution to me.
Here's the class definition.

* An abstraction on top of {@link HelixManager}, created for the {@link PinotTaskExecutor}, restricted to only * get/update minion task metadata

Let's brainstorm the better way (e.g. add the new method instead of changing the interface)

As discussed offline, based on the definition, the minion configs should not be maintained by this class. Moved the config into BaseMultipleSegmentsConversionExecutor class. Added new init function in PinotTaskExecutorFactory to make it backward compatible.

jackjlli · 2021-10-07T22:13:45Z

.../test/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskExecutorTest.java

@@ -104,7 +104,7 @@ public void setUp()
  @Test
  public void testConvert()
      throws Exception {
-    MergeRollupTaskExecutor mergeRollupTaskExecutor = new MergeRollupTaskExecutor();
+    MergeRollupTaskExecutor mergeRollupTaskExecutor = new MergeRollupTaskExecutor(null);


If the minionZkMetadataManager is null, can we just use the default constructor here?

Updated the constructor to use MinionConf as input instead. Hmm, this requires to add the default constructor explicitly while the config should be a required field. Maybe we can use new MinionConf() instead cause this field should never be null?

Sounds good.

jackjlli · 2021-10-07T22:17:56Z

pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java

+        _config.getProperty(CommonConstants.Minion.END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS_KEY,
+            CommonConstants.Minion.DEFAULT_END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS);
+    MinionTaskZkMetadataManager minionTaskZkMetadataManager =
+        new MinionTaskZkMetadataManager(_helixManager, endReplaceSegmentsSocketTimeoutMs);


How about passing the whole PinotConfiguration to the MinionTaskZkMetadataManager class? By doing so, you don't have to keep adding parameters to the constructor (which is a bad idea) if needed.

Good point! This is also mentioned by @snleee offline. Created the new MinionConf class instead. Based on the definition of MinionTaskZkMetadataManager, moved the configs out of this class.

…config instead of task config.

snleee

LGTM other than the minor comment.

snleee · 2021-10-08T03:44:54Z

pinot-minion/src/main/java/org/apache/pinot/minion/executor/PinotTaskExecutorFactory.java

@@ -26,8 +29,14 @@
  /**
   * Initializes the task executor factory.
   */
+  @Deprecated


Can you make sure there's no usage of this within the Pinot code base?

Yes, there's no usage of this function.

…config instead of task config. (apache#7516)

jackjlli reviewed Oct 4, 2021

View reviewed changes

jtao15 force-pushed the externalViewOnlineSegmentsTimeout branch 2 times, most recently from 1b3ec00 to 29c3d61 Compare October 5, 2021 00:50

snleee reviewed Oct 5, 2021

View reviewed changes

jackjlli reviewed Oct 7, 2021

View reviewed changes

jtao15 force-pushed the externalViewOnlineSegmentsTimeout branch from 29c3d61 to c4f9a4d Compare October 7, 2021 22:26

Introduce MinionConf, move END_REPLACE_SEGMENTS_TIMEOUT_MS to minion …

761e28c

…config instead of task config.

jtao15 force-pushed the externalViewOnlineSegmentsTimeout branch from c4f9a4d to 761e28c Compare October 8, 2021 00:31

snleee approved these changes Oct 8, 2021

View reviewed changes

snleee merged commit 6969b38 into apache:master Oct 8, 2021

kriti-sc pushed a commit to kriti-sc/incubator-pinot that referenced this pull request Dec 12, 2021

Introduce MinionConf, move END_REPLACE_SEGMENTS_TIMEOUT_MS to minion …

0a602b6

…config instead of task config. (apache#7516)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config. #7516

Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config. #7516

jtao15 commented Oct 4, 2021

jackjlli Oct 4, 2021

jtao15 Oct 5, 2021

jackjlli Oct 4, 2021

jtao15 Oct 5, 2021

codecov-commenter commented Oct 4, 2021 •

edited

Loading

snleee Oct 5, 2021

jtao15 Oct 7, 2021

jackjlli Oct 7, 2021

jtao15 Oct 7, 2021

jackjlli Oct 7, 2021

jackjlli Oct 7, 2021

jtao15 Oct 7, 2021

snleee left a comment

snleee Oct 8, 2021

jtao15 Oct 8, 2021


		public MinionTaskZkMetadataManager(HelixManager helixManager) {
		public MinionTaskZkMetadataManager(HelixManager helixManager, int endReplaceSegmentsSocketTimeoutMs) {

Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config. #7516

Move END_REPLACE_SEGMENTS_SOCKET_TIMEOUT_MS to minion config instead of task config. #7516

Conversation

jtao15 commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 4, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snleee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 4, 2021 •

edited

Loading