-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tiered storage #5793
Tiered storage #5793
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done.
Most comments are minor (comments in OfflineSegmentAssignment
also applies to RealtimeSegmentAssignment
).
Please merge the 2 relocators into one.
...t-common/src/main/java/org/apache/pinot/common/assignment/InstanceAssignmentConfigUtils.java
Outdated
Show resolved
Hide resolved
...t-common/src/main/java/org/apache/pinot/common/assignment/InstanceAssignmentConfigUtils.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TierConfig.java
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
Thanks for the review. I've merged the 2 relocators into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. One critical comment in SegmentRelocator
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java
Outdated
Show resolved
Hide resolved
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerStarter.java
Outdated
Show resolved
Hide resolved
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java
Show resolved
Hide resolved
...roller/src/main/java/org/apache/pinot/controller/helix/core/relocation/SegmentRelocator.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/pinot/integration/tests/ControllerPeriodicTasksIntegrationTest.java
Show resolved
Hide resolved
...c/main/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignment.java
Outdated
Show resolved
Hide resolved
...java/org/apache/pinot/controller/helix/core/assignment/segment/OfflineSegmentAssignment.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/pinot/controller/helix/core/assignment/segment/RealtimeSegmentAssignment.java
Outdated
Show resolved
Hide resolved
Made one small change not part of the review. Converted the "segmentSelectorType" and "storageType" to enums. If you want to look again. |
pinot-common/src/main/java/org/apache/pinot/common/tier/PinotServerTierStorage.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/tier/TierSegmentSelector.java
Show resolved
Hide resolved
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/TierConfig.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/config/TierConfigUtils.java
Show resolved
Hide resolved
Since we introduced the enum, try to use enum over string for these 2 fields |
pinot-common/src/main/java/org/apache/pinot/common/tier/PinotServerTierStorage.java
Outdated
Show resolved
Hide resolved
*/ | ||
public class PinotServerTierStorage implements TierStorage { | ||
private final String _type = TierStorageType.PINOT_SERVER.toString(); | ||
private final String _tag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we support multiple tags for a tier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a good idea. I can add that for phase 2 (where I'll be handling advanced instance assignments for tiers)
pinot-common/src/main/java/org/apache/pinot/common/tier/TierFactory.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/tier/TierFactory.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/tier/TierSegmentSelector.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/tier/TierStorage.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/tier/TimeBasedTierSegmentSelector.java
Outdated
Show resolved
Hide resolved
/** | ||
* Interface for the segment selection strategy of a tier | ||
*/ | ||
public interface TierSegmentSelector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking of adding a method like int getPriority()
?
For time based tiers we can use internal age for comparison if priority is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I'll add that when the requirements demand it.. As of now, I didn't see it being requirwd
...n/java/org/apache/pinot/controller/helix/core/assignment/segment/SegmentAssignmentUtils.java
Outdated
Show resolved
Hide resolved
The reason I kept it string is that if we want people to plug in their own strategies, they can do so without needing to add it to the enum |
In that case, IMO we should not introduce the enum |
Hmm, agreed. Reverted enum change. |
Description
Issue: #5553
Tiered storage support in Pinot - Phase 1.
This phase supports default tag based instance assignments only, which are not persisted in zk.
Phase 2 will introduce instanceAssignmentConfig for tiers, which will allow us to support replica groups for tiers and also let us persist the InstancePartitions
Example:
This example show how to configure segments older than 15 days move to
tier_b_OFFLINE
and segments older than 7 days move totier_a_OFFLINE
Release Notes
Tiered storage phase 1 - default tag based instance assignment for tiers