Skip to content

Conversation

@GSharayu
Copy link
Contributor

@GSharayu GSharayu commented May 4, 2022

Currently, we have SegmentAssignment interface and OfflineSegmentAssignment & RealtimeSegmentAssignment as its implementation. This approach doesn't allow us to pick the custom segment assignment strategy.

This PR provides SegmentAssignmentStrategy as an interface improvement and pre-requisite work.

Issue #8585

@GSharayu
Copy link
Contributor Author

GSharayu commented May 4, 2022

@snleee @sajjad-moradi

@snleee
Copy link
Contributor

snleee commented May 4, 2022

Current code is moving SegmentAssignment to SegmentAssignmentStrategy. Instead, we would like to separate out the logic for SegmentAssignmentStrategy from SegmentAssignment. Ideally, I think that the following structure is needed:

1. SegmentAssignementStrategy -> ReplicaGroupSegmentAssignmentStrategy, BalancedSegmentAssignmentStrategy, CostBasedSegmentAssignmentStrategy etc
2. SegmentAssignment -> OfflineSegmentAssignment, RealtimeSegmentAssignment, etc

Offline/RealtimeSegmentAssignment implementation should pick replica group vs balanced based or other assignment strategy implementation on the table config.

Current code requires the code modification directly to OfflineSegmentAssignement or RealtimeSegmentAssignment if we want to add a new type of assignment strategy. The goal should be making that pluggable.

Copy link
Member

@jackjlli jackjlli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feed that the current offline/realtime segment assignment classes should extend from the same abstract base class, and inside that base class different assignment strategy can be chosen.

* Factory for the {@link SegmentAssignment}.
* Factory for the {@link SegmentAssignmentStrategy}.
*/
public class SegmentAssignmentFactory {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sed 's/SegmentAssignmentFactory/SegmentAssignmentStrategyFactory/'

*/
public class OfflineSegmentAssignment implements SegmentAssignment {
private static final Logger LOGGER = LoggerFactory.getLogger(OfflineSegmentAssignment.class);
public class OfflineSegmentAssignmentStrategy implements SegmentAssignmentStrategy {
Copy link
Contributor

@siddharthteotia siddharthteotia May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem correct to me

OfflineSegmentAssignment or RealtimeSegmentAssignment is not a strategy imo

Regardless of offline v/s realtime, replica group and balanced are the concrete implementations of strategies

cc @jasperjiaguo since you are also going to touch related code later

@GSharayu GSharayu changed the title Pre-req work for SegmentAssignmentStrategy (#8585) [WIP]Pre-req work for SegmentAssignmentStrategy (#8585) May 5, 2022
@GSharayu GSharayu changed the title [WIP]Pre-req work for SegmentAssignmentStrategy (#8585) [WIP][Not Ready for Review]Pre-req work for SegmentAssignmentStrategy (#8585) May 5, 2022
@Jackie-Jiang
Copy link
Contributor

I don't see the difference of the new SegmentAssignmentStrategy and the existing SegmentAssignment. If you want to plug-in custom assignment, just add the support to SegmentAssignmentFactory

@GSharayu GSharayu closed this Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants