You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "abhioncbr (via GitHub)" <gi...@apache.org> on 2023/05/24 02:16:33 UTC
[GitHub] [pinot] abhioncbr opened a new pull request, #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
abhioncbr opened a new pull request, #10656:
URL: https://github.com/apache/pinot/pull/10656
- Adding the `partitionColumn` field in `InstanceReplicaGroupPartitionConfig`
`labels`:
- refactor
- cleanup
Issue https://github.com/apache/pinot/issues/10608
--
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] abhioncbr commented on pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1534023782
Please review it once more. Thanks
--
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] abhioncbr commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1204607591
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +447,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (String key : tableConfig.getInstanceAssignmentConfigMap().keySet()) {
+ //check getInstanceAssignmentConfigMap has the key of TableType
+ if (Objects.equals(key, tableConfig.getTableType().toString())) {
Review Comment:
I have made the changes. Thanks
--
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] Jackie-Jiang commented on pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1526119626
> Question for `InstanceAssignmentConfigUtils`, since we are creating an object of `InstanceReplicaGroupPartitionConfig`[L116, L121] and with this change we have added `partitionColumn` as a class member, we need to provide the value of it. I thought it's better to give the value we are extracting there instead of setting it as `null`. WDYT?
Good point. We should set the `partitionColumn` in the generated `InstanceReplicaGroupPartitionConfig`
--
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] abhioncbr commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1183115927
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/utils/SegmentUtils.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.helix.core.assignment.utils;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.config.table.ReplicaGroupStrategyConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+public class SegmentUtils {
+ private SegmentUtils() {
+ }
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+/* // check getInstanceAssignmentConfigMap is null or empty,
Review Comment:
I need help here.
Actually, as per my understanding, this commented code should be needed to get the `partitionColumn` value from the `InstanceReplicaGroupPartitionConfig`, but it results in failure which running individually passes.
--
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] abhioncbr commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1183115927
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/utils/SegmentUtils.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.helix.core.assignment.utils;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.config.table.ReplicaGroupStrategyConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+public class SegmentUtils {
+ private SegmentUtils() {
+ }
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+/* // check getInstanceAssignmentConfigMap is null or empty,
Review Comment:
I need help here.
Actually, as per my understanding, this commented code should be needed to get the `partitionColumn` value from the `InstanceReplicaGroupPartitionConfig`, but it results in failure which running individually passes.
--
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] abhioncbr commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1207501501
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +446,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from tableConfig instance assignment config map.
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (Map.Entry<String, InstanceAssignmentConfig> entry: tableConfig.getInstanceAssignmentConfigMap().entrySet()) {
+ InstanceAssignmentConfig instanceAssignmentConfig = entry.getValue();
+ //check InstanceAssignmentConfig has the InstanceReplicaGroupPartitionConfig with non-empty partitionColumn
+ if (StringUtils.isNotEmpty(instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn())) {
+ // if true, set the partitionColumn value.
+ partitionColumn = instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn();
+ }
+ }
+ }
+
+ // check, if partitionColumn is not empty, return the value.
+ if (!StringUtils.isEmpty(partitionColumn)) {
+ return partitionColumn;
+ }
Review Comment:
Thanks, changing it as per the suggestion.
--
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] Jackie-Jiang commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1207506425
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +446,29 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from tableConfig instance assignment config map.
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
Review Comment:
```suggestion
// check InstanceAssignmentConfigMap is null or empty,
```
--
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] Jackie-Jiang merged pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10656:
URL: https://github.com/apache/pinot/pull/10656
--
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] Jackie-Jiang commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1207487758
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +446,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from tableConfig instance assignment config map.
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (Map.Entry<String, InstanceAssignmentConfig> entry: tableConfig.getInstanceAssignmentConfigMap().entrySet()) {
+ InstanceAssignmentConfig instanceAssignmentConfig = entry.getValue();
+ //check InstanceAssignmentConfig has the InstanceReplicaGroupPartitionConfig with non-empty partitionColumn
+ if (StringUtils.isNotEmpty(instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn())) {
+ // if true, set the partitionColumn value.
+ partitionColumn = instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn();
+ }
+ }
+ }
+
+ // check, if partitionColumn is not empty, return the value.
+ if (!StringUtils.isEmpty(partitionColumn)) {
+ return partitionColumn;
+ }
Review Comment:
Can be simplified
```suggestion
// check InstanceAssignmentConfigMap is null or empty,
if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
for (InstanceAssignmentConfig instanceAssignmentConfig : tableConfig.getInstanceAssignmentConfigMap().values()) {
//check InstanceAssignmentConfig has the InstanceReplicaGroupPartitionConfig with non-empty partitionColumn
if (StringUtils.isNotEmpty(instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn())) {
return instanceAssignmentConfig.getReplicaGroupPartitionConfig().getPartitionColumn();
}
}
}
```
--
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 #10656: 10608: Changes for removing ReplicaGroupStrategyConfig
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1517061450
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10656](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6b9a9aa) into [master](https://codecov.io/gh/apache/pinot/commit/7adac6f60eb6bafc98eb0f61f4860fadf40f4f03?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7adac6f) will **decrease** coverage by `56.49%`.
> The diff coverage is `11.11%`.
```diff
@@ Coverage Diff @@
## master #10656 +/- ##
=============================================
- Coverage 70.35% 13.86% -56.49%
+ Complexity 6499 439 -6060
=============================================
Files 2107 2053 -54
Lines 113460 111043 -2417
Branches 17101 16816 -285
=============================================
- Hits 79822 15398 -64424
- Misses 28038 94387 +66349
+ Partials 5600 1258 -4342
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.86% <11.11%> (-0.02%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...mmon/assignment/InstanceAssignmentConfigUtils.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZUFzc2lnbm1lbnRDb25maWdVdGlscy5qYXZh) | `0.00% <0.00%> (-10.26%)` | :arrow_down: |
| [...ig/table/SegmentsValidationAndRetentionConfig.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1NlZ21lbnRzVmFsaWRhdGlvbkFuZFJldGVudGlvbkNvbmZpZy5qYXZh) | `0.00% <0.00%> (-96.08%)` | :arrow_down: |
| [...ssignment/InstanceReplicaGroupPartitionConfig.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL2Fzc2lnbm1lbnQvSW5zdGFuY2VSZXBsaWNhR3JvdXBQYXJ0aXRpb25Db25maWcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...he/pinot/spi/utils/builder/TableConfigBuilder.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvYnVpbGRlci9UYWJsZUNvbmZpZ0J1aWxkZXIuamF2YQ==) | `0.00% <0.00%> (-87.28%)` | :arrow_down: |
| [...core/assignment/segment/BaseSegmentAssignment.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvc2VnbWVudC9CYXNlU2VnbWVudEFzc2lnbm1lbnQuamF2YQ==) | `97.72% <100.00%> (-0.10%)` | :arrow_down: |
| [...trategy/ReplicaGroupSegmentAssignmentStrategy.java](https://codecov.io/gh/apache/pinot/pull/10656?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvc2VnbWVudC9zdHJhdGVneS9SZXBsaWNhR3JvdXBTZWdtZW50QXNzaWdubWVudFN0cmF0ZWd5LmphdmE=) | `90.56% <100.00%> (-0.35%)` | :arrow_down: |
... and [1662 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10656/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
: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=The+Apache+Software+Foundation)
--
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] Jackie-Jiang commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1203126876
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +447,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
Review Comment:
(minor) Update the javadoc
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +447,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (String key : tableConfig.getInstanceAssignmentConfigMap().keySet()) {
Review Comment:
(minor) Avoid extra map lookup by using `entrySet()`
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +447,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (String key : tableConfig.getInstanceAssignmentConfigMap().keySet()) {
+ //check getInstanceAssignmentConfigMap has the key of TableType
+ if (Objects.equals(key, tableConfig.getTableType().toString())) {
Review Comment:
The key of the map is not always the table type. We can simply loop over the map (`values()`), and return the partition column if it is configured
--
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] abhioncbr closed pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr closed pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
URL: https://github.com/apache/pinot/pull/10656
--
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] abhioncbr commented on pull request #10656: 10608: Changes for removing ReplicaGroupStrategyConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1517047083
> This is backwards incompatible. Why do we want to do this without first ensuring / aligning that everyone has their production deployment migrated to new style replica group config ?
Thanks for mentioning it; I didn't consider backward compatibility while making the changes. So, to make this backward compatible, we can't delete the `ReplicaGroupStrategyConfig` class.
As the first step, can we add the `partitionColumn` field to the `InstanceReplicaGroupPartitionConfig` class? However, what if a user provides a different `partitionColumn` value in both configs? Which one would be applicable, `ReplicaGroupStrategyConfig` or `InstanceReplicaGroupPartitionConfig`?
Also, please give pointers on some of the factors I should consider for making the change backward compatible. Thanks
--
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] abhioncbr commented on pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1524327572
> I don't think this is correct. We don't need to change anything in `InstanceAssignmentConfigUtils` because it will directly return the new config if it exists (line 95). `ReplicaGroupStrategyConfig.getPartitionColumn()` is used in `BaseSegmentAssignment` and `ReplicaGroupSegmentAssignmentStrategy`, so we should modify these 2 classes to be able to take the partition column from the new config
Thanks for the review. Going to make the changes in `BaseSegmentAssignment` and `ReplicaGroupSegmentAssignmentStrategy`.
Question for `InstanceAssignmentConfigUtils`, since we are creating an object of `InstanceReplicaGroupPartitionConfig`[L116, L121] and with this change we have added `partitionColumn` as a class member, we need to provide the value of it. I thought it's better to give the value we are extracting there instead of setting it as `null`. WDYT?
--
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] ankitsultana commented on pull request #10656: 10608: Changes for removing ReplicaGroupStrategyConfig
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1517101707
+1 to @siddharthteotia 's comment.
I'd encourage for any backwards incompatible change there should be a small doc which outlines how users can migrate and the motivation behind the same.
--
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] siddharthteotia commented on pull request #10656: 10608: Changes for removing ReplicaGroupStrategyConfig
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1516973176
This is backwards incompatible. Why do we want to do this without first ensuring / aligning that everyone has their production deployment migrated to new style replica group config ?
--
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] abhioncbr commented on pull request #10656: 10608: Changes for removing ReplicaGroupStrategyConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on PR #10656:
URL: https://github.com/apache/pinot/pull/10656#issuecomment-1518923337
Please review. Changes are as per the [discussion](https://github.com/apache/pinot/issues/10608).
Thanks
--
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] Jackie-Jiang commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1182093501
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/utils/SegmentUtils.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.helix.core.assignment.utils;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.config.table.ReplicaGroupStrategyConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+public class SegmentUtils {
+ private SegmentUtils() {
+ }
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
Review Comment:
I feel this can be added to the `TableConfigUtils` (the one under `pinot-common`)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/utils/SegmentUtils.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.helix.core.assignment.utils;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.pinot.spi.config.table.ReplicaGroupStrategyConfig;
+import org.apache.pinot.spi.config.table.TableConfig;
+
+public class SegmentUtils {
+ private SegmentUtils() {
+ }
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+/* // check getInstanceAssignmentConfigMap is null or empty,
Review Comment:
Please clean this up
##########
pinot-common/src/main/java/org/apache/pinot/common/assignment/InstanceAssignmentConfigUtils.java:
##########
@@ -109,18 +109,16 @@ public static InstanceAssignmentConfig getInstanceAssignmentConfig(TableConfig t
Preconditions.checkState(replicaGroupStrategyConfig != null, "Failed to find the replica-group strategy config");
String partitionColumn = replicaGroupStrategyConfig.getPartitionColumn();
boolean minimizeDataMovement = segmentConfig.isMinimizeDataMovement();
+ int numPartitions = 0;
if (partitionColumn != null) {
- int numPartitions = tableConfig.getIndexingConfig().getSegmentPartitionConfig().getNumPartitions(partitionColumn);
+ numPartitions = tableConfig.getIndexingConfig().getSegmentPartitionConfig().getNumPartitions(partitionColumn);
Preconditions.checkState(numPartitions > 0, "Number of partitions for column: %s is not properly configured",
partitionColumn);
- replicaGroupPartitionConfig = new InstanceReplicaGroupPartitionConfig(true, 0, numReplicaGroups, 0, numPartitions,
- replicaGroupStrategyConfig.getNumInstancesPerPartition(), minimizeDataMovement);
- } else {
- // If partition column is not configured, use replicaGroupStrategyConfig.getNumInstancesPerPartition() as
- // number of instances per replica-group for backward-compatibility
- replicaGroupPartitionConfig = new InstanceReplicaGroupPartitionConfig(true, 0, numReplicaGroups,
- replicaGroupStrategyConfig.getNumInstancesPerPartition(), 0, 0, minimizeDataMovement);
}
+ // If partition column is not configured, use replicaGroupStrategyConfig.getNumInstancesPerPartition() as
Review Comment:
This changes the behavior. We can keep the current code, and just put `partitionName` to the generated `InstanceReplicaGroupPartitionConfig` in the if branch. In the else branch, simply put `null` as `partitionName`
--
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] abhioncbr commented on a diff in pull request #10656: 10608: Changes for adding partitionColumn in replicaGroupPartitionConfig
Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10656:
URL: https://github.com/apache/pinot/pull/10656#discussion_r1204607591
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TableConfigUtils.java:
##########
@@ -444,4 +447,36 @@ public static boolean hasPreConfiguredInstancePartitions(TableConfig tableConfig
return hasPreConfiguredInstancePartitions(tableConfig)
&& tableConfig.getInstancePartitionsMap().containsKey(instancePartitionsType);
}
+
+ /**
+ * Get the partition column from InstanceAssignmentConfigUtils
+ * @param tableConfig table config
+ * @return partition column
+ */
+ public static String getPartitionColumn(TableConfig tableConfig) {
+ String partitionColumn = null;
+
+ // check getInstanceAssignmentConfigMap is null or empty,
+ if (!MapUtils.isEmpty(tableConfig.getInstanceAssignmentConfigMap())) {
+ for (String key : tableConfig.getInstanceAssignmentConfigMap().keySet()) {
+ //check getInstanceAssignmentConfigMap has the key of TableType
+ if (Objects.equals(key, tableConfig.getTableType().toString())) {
Review Comment:
It is updated as per the comments. Please have another look. Thanks
--
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