klsince commented on code in PR #10746:
URL: https://github.com/apache/pinot/pull/10746#discussion_r1188912139
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java:
##########
@@ -63,6 +67,32 @@ public static String getDataDirForTier(TableConfig
tableConfig, String tierName)
return getDataDirForTier(tableConfig, tierName, Collections.emptyMap());
}
+ /**
+ * Consider configured tiers and compute default instance partitions for the
segment
+ *
+ * @return InstancePartitions if the one can be derived from the given
sorted tiers, null otherwise
+ */
+ @Nullable
+ public static InstancePartitions
getTieredInstancePartitionsForSegment(String tableNameWithType,
+ @Nullable List<Tier> sortedTiers, String segmentName, HelixManager
helixManager) {
Review Comment:
nit: adjust the order of params a bit like `String tableNameWithType, String
segmentName, ... `
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java:
##########
@@ -32,16 +32,20 @@
import
org.apache.pinot.controller.helix.core.assignment.segment.strategy.SegmentAssignmentStrategyFactory;
import org.apache.pinot.spi.config.table.assignment.InstancePartitionsType;
import org.apache.pinot.spi.utils.RebalanceConfigConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Segment assignment for offline table.
*/
public class OfflineSegmentAssignment extends BaseSegmentAssignment {
+ private final Logger _logger = LoggerFactory.getLogger(getClass());
Review Comment:
I believe we can remove those changes in this class
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -2268,10 +2271,30 @@ public void assignTableSegment(String
tableNameWithType, String segmentName) {
try {
TableConfig tableConfig = getTableConfig(tableNameWithType);
Preconditions.checkState(tableConfig != null, "Failed to find table
config for table: " + tableNameWithType);
+
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap =
fetchOrComputeInstancePartitions(tableNameWithType, tableConfig);
+
+ // Initialize tier information only in case direct tier assignment is
configured
+ if (_enableTieredSegmentAssignment &&
CollectionUtils.isNotEmpty(tableConfig.getTierConfigsList())) {
+ List<Tier> sortedTiers =
TierConfigUtils.getSortedTiersForStorageType(tableConfig.getTierConfigsList(),
+ TierFactory.PINOT_SERVER_STORAGE_TYPE, _helixZkManager);
+
+ // Update segment tier to support direct assignment for multiple data
directories
+ updateSegmentTargetTier(tableNameWithType, segmentName, sortedTiers);
+
+ InstancePartitions tierInstancePartitions =
+
TierConfigUtils.getTieredInstancePartitionsForSegment(tableNameWithType,
sortedTiers, segmentName,
+ _helixZkManager);
+ if (tierInstancePartitions != null &&
TableNameBuilder.isOfflineTableResource(tableNameWithType)) {
+ // Override instance partitions for offline table
Review Comment:
maybe change this comment to an INFO or DEBUG log to help troubleshooting in
case?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/segment/RealtimeSegmentAssignment.java:
##########
@@ -34,6 +34,8 @@
import org.apache.pinot.spi.config.table.assignment.InstancePartitionsType;
import
org.apache.pinot.spi.utils.CommonConstants.Helix.StateModel.SegmentStateModel;
import org.apache.pinot.spi.utils.RebalanceConfigConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
Review Comment:
and changes in this class are not needed too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
