You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jtao15 (via GitHub)" <gi...@apache.org> on 2023/07/13 17:29:21 UTC

[GitHub] [pinot] jtao15 opened a new pull request, #11101: Allow setting the default MergeRollupTask segment group manager

jtao15 opened a new pull request, #11101:
URL: https://github.com/apache/pinot/pull/11101

   This pr allows setting the default segment group manager for MergeRollupTask globally. Table level group manager specified in the task configs will override the default one.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] jtao15 commented on a diff in pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #11101:
URL: https://github.com/apache/pinot/pull/11101#discussion_r1262893753


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/segmentgroupmananger/MergeRollupTaskSegmentGroupManagerProvider.java:
##########
@@ -27,20 +27,27 @@
  * Provider class for {@link MergeRollupTaskSegmentGroupManager}
  */
 public abstract class MergeRollupTaskSegmentGroupManagerProvider {
+
+  private static String _defaultMergeRollupTaskSegmentGroupManagerClassName =
+      "DefaultMergeRollupTaskSegmentGroupManager";
+
+  public static void setDefaultMergeRollupTaskSegmentGroupManagerClassName(String className) {
+    _defaultMergeRollupTaskSegmentGroupManagerClassName = className;
+  }
+
   /**
    * Constructs the {@link MergeRollupTaskSegmentGroupManager} using MergeRollup task configs
    */
   public static MergeRollupTaskSegmentGroupManager create(Map<String, String> taskConfigs) {
     String segmentGroupManagerClassName =
         taskConfigs.get(MinionConstants.MergeRollupTask.SEGMENT_GROUP_MANAGER_CLASS_NAME_KEY);

Review Comment:
   Updated.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] sajjad-moradi commented on a diff in pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "sajjad-moradi (via GitHub)" <gi...@apache.org>.
sajjad-moradi commented on code in PR #11101:
URL: https://github.com/apache/pinot/pull/11101#discussion_r1262882994


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/segmentgroupmananger/MergeRollupTaskSegmentGroupManagerProvider.java:
##########
@@ -27,20 +27,27 @@
  * Provider class for {@link MergeRollupTaskSegmentGroupManager}
  */
 public abstract class MergeRollupTaskSegmentGroupManagerProvider {
+
+  private static String _defaultMergeRollupTaskSegmentGroupManagerClassName =
+      "DefaultMergeRollupTaskSegmentGroupManager";

Review Comment:
   Doesn't the plugin require fully qualified class name?



##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/segmentgroupmananger/MergeRollupTaskSegmentGroupManagerProvider.java:
##########
@@ -27,20 +27,27 @@
  * Provider class for {@link MergeRollupTaskSegmentGroupManager}
  */
 public abstract class MergeRollupTaskSegmentGroupManagerProvider {
+
+  private static String _defaultMergeRollupTaskSegmentGroupManagerClassName =
+      "DefaultMergeRollupTaskSegmentGroupManager";
+
+  public static void setDefaultMergeRollupTaskSegmentGroupManagerClassName(String className) {
+    _defaultMergeRollupTaskSegmentGroupManagerClassName = className;
+  }
+
   /**
    * Constructs the {@link MergeRollupTaskSegmentGroupManager} using MergeRollup task configs
    */
   public static MergeRollupTaskSegmentGroupManager create(Map<String, String> taskConfigs) {
     String segmentGroupManagerClassName =
         taskConfigs.get(MinionConstants.MergeRollupTask.SEGMENT_GROUP_MANAGER_CLASS_NAME_KEY);

Review Comment:
   Use `.getOrDefault` instead.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] PrachiKhobragade commented on a diff in pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "PrachiKhobragade (via GitHub)" <gi...@apache.org>.
PrachiKhobragade commented on code in PR #11101:
URL: https://github.com/apache/pinot/pull/11101#discussion_r1262917809


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/segmentgroupmananger/MergeRollupTaskSegmentGroupManagerProvider.java:
##########
@@ -27,20 +27,25 @@
  * Provider class for {@link MergeRollupTaskSegmentGroupManager}
  */
 public abstract class MergeRollupTaskSegmentGroupManagerProvider {
+
+  private static String _defaultMergeRollupTaskSegmentGroupManagerClassName =
+      "org.apache.pinot.plugin.minion.tasks.mergerollup.segmentgroupmananger.DefaultMergeRollupTaskSegmentGroupManager";

Review Comment:
   Better way would be class.getName(), so that any refactoring efforst won't miss this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] sajjad-moradi merged pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "sajjad-moradi (via GitHub)" <gi...@apache.org>.
sajjad-moradi merged PR #11101:
URL: https://github.com/apache/pinot/pull/11101


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] codecov-commenter commented on pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11101:
URL: https://github.com/apache/pinot/pull/11101#issuecomment-1634712919

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11101?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#11101](https://app.codecov.io/gh/apache/pinot/pull/11101?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c56dc2a) into [master](https://app.codecov.io/gh/apache/pinot/commit/1622a9e7d689537881826549a9323237ca7d7e44?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1622a9e) will **increase** coverage by `0.00%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@            Coverage Diff            @@
   ##           master   #11101     +/-   ##
   =========================================
     Coverage    0.11%    0.11%             
   =========================================
     Files        2203     2148     -55     
     Lines      118165   115645   -2520     
     Branches    17879    17572    -307     
   =========================================
     Hits          137      137             
   + Misses     118008   115488   -2520     
     Partials       20       20             
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1temurin11 | `?` | |
   | integration1temurin17 | `?` | |
   | integration1temurin20 | `?` | |
   | integration2temurin11 | `?` | |
   | integration2temurin17 | `?` | |
   | unittests1temurin11 | `?` | |
   | unittests1temurin17 | `?` | |
   | unittests1temurin20 | `?` | |
   | unittests2temurin11 | `0.11% <0.00%> (ø)` | |
   | unittests2temurin17 | `?` | |
   | unittests2temurin20 | `0.11% <0.00%> (ø)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...er/MergeRollupTaskSegmentGroupManagerProvider.java](https://app.codecov.io/gh/apache/pinot/pull/11101?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvbWVyZ2Vyb2xsdXAvc2VnbWVudGdyb3VwbWFuYW5nZXIvTWVyZ2VSb2xsdXBUYXNrU2VnbWVudEdyb3VwTWFuYWdlclByb3ZpZGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   
   ... and [55 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11101/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] jtao15 commented on a diff in pull request #11101: Allow setting the default MergeRollupTask segment group manager

Posted by "jtao15 (via GitHub)" <gi...@apache.org>.
jtao15 commented on code in PR #11101:
URL: https://github.com/apache/pinot/pull/11101#discussion_r1262893955


##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/segmentgroupmananger/MergeRollupTaskSegmentGroupManagerProvider.java:
##########
@@ -27,20 +27,27 @@
  * Provider class for {@link MergeRollupTaskSegmentGroupManager}
  */
 public abstract class MergeRollupTaskSegmentGroupManagerProvider {
+
+  private static String _defaultMergeRollupTaskSegmentGroupManagerClassName =
+      "DefaultMergeRollupTaskSegmentGroupManager";

Review Comment:
   Good catch! updated.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org