You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/03/24 22:50:58 UTC
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8389: FixedSegmentSelector for tier configs
Jackie-Jiang commented on a change in pull request #8389:
URL: https://github.com/apache/pinot/pull/8389#discussion_r834794872
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java
##########
@@ -76,20 +76,33 @@ public static boolean shouldRelocateToTiers(TableConfig tableConfig) {
}
/**
- * Comparator for sorting the {@link Tier}.
- * As of now, we have only 1 type of {@link TierSegmentSelector} and 1 type of {@link TierStorage}.
- * Tier with an older age bucket in {@link TimeBasedTierSegmentSelector} should appear before a younger age bucket,
- * in sort order
- * TODO: As we add more types, this logic needs to be upgraded
+ * Comparator for sorting the {@link Tier}. In the sort order,
+ * 1) {@link FixedTierSegmentSelector} are always before others
+ * 2) For {@link TimeBasedTierSegmentSelector}, tiers with an older age bucket appear before a younger age bucket,
*/
public static Comparator<Tier> getTierComparator() {
return (o1, o2) -> {
TierSegmentSelector s1 = o1.getSegmentSelector();
TierSegmentSelector s2 = o2.getSegmentSelector();
- Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType()),
+
+ Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType())
+ || TierFactory.FIXED_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType()),
"Unsupported segmentSelectorType class %s", s1.getClass());
- Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType()),
+ Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType())
+ || TierFactory.FIXED_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType()),
"Unsupported segmentSelectorType class %s", s2.getClass());
+
+ if (TierFactory.FIXED_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType())
Review comment:
(minor) You may do `equals` (even `==` works)
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java
##########
@@ -76,20 +76,33 @@ public static boolean shouldRelocateToTiers(TableConfig tableConfig) {
}
/**
- * Comparator for sorting the {@link Tier}.
- * As of now, we have only 1 type of {@link TierSegmentSelector} and 1 type of {@link TierStorage}.
- * Tier with an older age bucket in {@link TimeBasedTierSegmentSelector} should appear before a younger age bucket,
- * in sort order
- * TODO: As we add more types, this logic needs to be upgraded
+ * Comparator for sorting the {@link Tier}. In the sort order,
+ * 1) {@link FixedTierSegmentSelector} are always before others
+ * 2) For {@link TimeBasedTierSegmentSelector}, tiers with an older age bucket appear before a younger age bucket,
*/
public static Comparator<Tier> getTierComparator() {
return (o1, o2) -> {
TierSegmentSelector s1 = o1.getSegmentSelector();
TierSegmentSelector s2 = o2.getSegmentSelector();
- Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType()),
+
+ Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType())
+ || TierFactory.FIXED_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s1.getType()),
"Unsupported segmentSelectorType class %s", s1.getClass());
- Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType()),
+ Preconditions.checkState(TierFactory.TIME_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType())
+ || TierFactory.FIXED_SEGMENT_SELECTOR_TYPE.equalsIgnoreCase(s2.getType()),
"Unsupported segmentSelectorType class %s", s2.getClass());
Review comment:
(minor) These checks are redundant. We don't really need to validate the `type` of a selector
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java
##########
@@ -76,20 +76,33 @@ public static boolean shouldRelocateToTiers(TableConfig tableConfig) {
}
/**
- * Comparator for sorting the {@link Tier}.
- * As of now, we have only 1 type of {@link TierSegmentSelector} and 1 type of {@link TierStorage}.
- * Tier with an older age bucket in {@link TimeBasedTierSegmentSelector} should appear before a younger age bucket,
- * in sort order
- * TODO: As we add more types, this logic needs to be upgraded
+ * Comparator for sorting the {@link Tier}. In the sort order,
+ * 1) {@link FixedTierSegmentSelector} are always before others
+ * 2) For {@link TimeBasedTierSegmentSelector}, tiers with an older age bucket appear before a younger age bucket,
Review comment:
(minor) remove the `,` in the end
--
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