-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Auto reset 2/3]Introduce ephemeral topic type into multi-topic ingestion #16494
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
[Auto reset 2/3]Introduce ephemeral topic type into multi-topic ingestion #16494
Conversation
@Jackie-Jiang @noob-se7en @KKcorps please help review, thanks. |
9091900
to
6e3771a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16494 +/- ##
============================================
- Coverage 63.34% 63.34% -0.01%
- Complexity 1379 1380 +1
============================================
Files 3027 3027
Lines 176446 176527 +81
Branches 27088 27101 +13
============================================
+ Hits 111765 111816 +51
- Misses 56115 56133 +18
- Partials 8566 8578 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f379825
to
be784a3
Compare
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Show resolved
Hide resolved
public int getPartitionGroupId() { | ||
return _partitionGroupId; | ||
} | ||
|
||
public String getPartitionGroupInfo() { |
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.
This can cause confusion. Can you move the topic name check to caller side?
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.
Change the function name to make it more intuitive
ff2b1ef
to
0835b15
Compare
@lnbest0707-uber can you please add problem and solution description just specific to this PR in the PR description.
|
The PR description is exactly for this PR only. You can forget about the offset auto reset story. The PR is about multi-topic ingestion. Other benefits for this change are in the description. |
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.
Pull Request Overview
This PR introduces support for ephemeral topic types in multi-topic ingestion as part of auto reset functionality. The main purpose is to enable different segment naming patterns for ephemeral backfill topics while maintaining compatibility with existing multi-topic ingestion.
Key changes include:
- Introduction of ephemeral backfill topic configuration and handling
- Modified segment naming to include topic names for ephemeral topics (format:
tableName__topicName__partitionGroupId__sequenceNumber__creationTime
) - Enhanced stream configuration handling to prevent duplicate topics and ensure proper configuration selection
- Updated partition group metadata handling to support topic-aware operations
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
IngestionConfigUtils.java | Added ephemeral topic partition offset and stream config selection method |
StreamConfigProperties.java | Added ephemeral backfill topic configuration property |
StreamConfig.java | Added ephemeral topic flag field with getter method |
PartitionGroupMetadataFetcher.java | Enhanced fetching logic to handle ephemeral topics differently |
PartitionGroupMetadata.java | Added topic name field and partition group identifier methods |
PartitionGroupConsumptionStatus.java | Added topic name field to consumption status tracking |
StreamIngestionConfig.java | Added realtime offset auto reset handler class configuration |
TableConfigUtils.java | Added validation to prevent duplicate topic names in stream configs |
LLCSegmentName.java | Extended to support topic names in segment naming format |
Various other files | Updated to handle topic-aware partition group operations |
@@ -431,6 +435,10 @@ public long getOffsetAutoResetTimeSecThreshold() { | |||
return _offsetAutoResetTimeSecThreshold; | |||
} | |||
|
|||
public Boolean isEphemeralBackfillTopic() { | |||
return Boolean.TRUE.equals(_ephemeralBackfillTopic); |
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.
The return statement uses Boolean.TRUE.equals() which is unnecessarily complex. Since _ephemeralBackfillTopic is initialized to false when null, a simple null check and return would be clearer: return _ephemeralBackfillTopic != null && _ephemeralBackfillTopic;
return Boolean.TRUE.equals(_ephemeralBackfillTopic); | |
return _ephemeralBackfillTopic != null && _ephemeralBackfillTopic; |
Copilot uses AI. Check for mistakes.
pinot-spi/src/main/java/org/apache/pinot/spi/stream/PartitionGroupMetadataFetcher.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
.map(String::trim) | ||
.map(Integer::parseInt) | ||
.collect(Collectors.toSet()); | ||
Set<String> targetSegments = allConsumingSegments.stream() | ||
.filter(segmentName -> partitionsToCommit.contains(new LLCSegmentName(segmentName).getPartitionGroupId())) | ||
.filter( | ||
segmentName -> partitionsToCommit.contains(new LLCSegmentName(segmentName).getPartitionGroupTopicAndId())) |
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.
The filter is calling getPartitionGroupTopicAndId() but comparing against partitionsToCommit which contains Integer values parsed from comma-separated string. This will cause ClassCastException since String cannot be compared to Integer.
segmentName -> partitionsToCommit.contains(new LLCSegmentName(segmentName).getPartitionGroupTopicAndId())) | |
segmentName -> partitionsToCommit.contains(new LLCSegmentName(segmentName).getPartitionGroupId())) |
Copilot uses AI. Check for mistakes.
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/utils/LLCSegmentName.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* Sets up a new partition group. | ||
* <p>Persists the ZK metadata for the first CONSUMING segment, and returns the segment name. | ||
*/ | ||
private String setupNewPartitionGroup(TableConfig tableConfig, StreamConfig streamConfig, | ||
private String setupNewPartitionGroup(TableConfig tableConfig, List<StreamConfig> streamConfigs, |
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 don't follow this change. What does it mean to setup a new partition group on multiple StreamConfig
? Which StreamConfig
should we reference when setting it up?
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.
If you check the usage of setupNewPartitionGroup
, they were all doing setupNewPartitionGroup(xx, streamConfigs.get(0), xxx)
.
The function mainly relies on partitionGroupMetadata
to setup new partition. And the streamConfig
is only used for fetching the flush threshold parameters. And it is using the 1st config's instead of the corresponding's.
With this change, we would pass in the entire table config ('s streamConfigMaps). And the followup usages can pick the correct streamConfig
by cross checking the partitionGroupMetadata
and use their own flush threshold values.
@@ -2776,54 +2842,4 @@ public List<String> getCommittingSegments(String realtimeTableName) { | |||
private List<String> getCommittingSegments(String realtimeTableName, Collection<String> segmentsToCheck) { | |||
return getCommittingSegments(realtimeTableName, segmentsToCheck, _helixResourceManager::getSegmentZKMetadata); | |||
} | |||
|
|||
public static List<String> getCommittingSegments(String realtimeTableName, |
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.
Several methods/comments are moved. What's the reason? Seems these methods fit more in their original position
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.
The IDE moved the static methods before non-static ones. We do not have strict enforcement in Pinot styles. If preferred not to be change, I can move them back.
List<PartitionGroupConsumptionStatus> topicPartitionGroupConsumptionStatusList = | ||
_partitionGroupConsumptionStatusList.stream() | ||
.filter(partitionGroupConsumptionStatus -> IngestionConfigUtils.getStreamConfigIndexFromPinotPartitionId( | ||
partitionGroupConsumptionStatus.getPartitionGroupId()) == index) | ||
.filter(partitionGroupConsumptionStatus -> _streamConfigs.get(index).isEphemeralBackfillTopic() |
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.
Can we move this check to PinotLLCRealtimeSegmentManager.setupNewPartitionGroup()
? Currently it is deeply nested, which makes it very hard to follow (I missed it several times, and in the end find this rewrite)
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.
Agree. This code is hard to follow. I've refactored it to completely split the permanent and ephemeral.
Also this class was with only simple functionality and without UT. I've also added extra UTs to test single/mutiple w/ and w/o ephemeral topics cases.
pinot-spi/src/main/java/org/apache/pinot/spi/stream/PartitionGroupMetadataFetcher.java
Outdated
Show resolved
Hide resolved
.filter(partitionGroupConsumptionStatus -> IngestionConfigUtils.getStreamConfigIndexFromPinotPartitionId( | ||
partitionGroupConsumptionStatus.getPartitionGroupId()) == index) | ||
.filter(partitionGroupConsumptionStatus -> _streamConfigs.get(index).isEphemeralBackfillTopic() | ||
? _streamConfigs.get(index).getTopicName().equals(partitionGroupConsumptionStatus.getTopicName()) |
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.
Do we need this check?
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.
Code refactored, pls check again, thanks
Given most of this PR is related to segment name format change, plus the fact that LLC segment format is used and checked in many Pinot classes. Is it possible to continue the current format but instead of encode the topic name in the current format? Put topic names in the segment name also can result in segment name length explosion and related metadata increase. One possible encoding scheme is to encode topic name, backfill id and partition id in the max allowed int 2,147,483,647 limit. |
I can think about 2 encoding methods but neither could fulfill the requirement
|
The backfill topic id is encoded in the partitioned id today. E.g., 11010005 means the 5th partition of the 11th topic (in the topic map) and it is a backfill topic (so 01). This is the way we can detect a backfill topic.
|
With this naming format, if we remove a topic, it might affect other topic's pinot partition naming format.
As above example, the issue is about removing topics. This index mapping makes topic partition ID rely on its position. And when other topics got removed, the position would change. |
Added an alternative solution in #16692 |
Abandon this one as the other option in #16692 is preferred. |
real-time
ingestion
feature
Part 2 of #15782, but this is independent from part 1
Issue #14815
Design doc https://docs.google.com/document/d/1NKPeNh6V2ctaQ4T_X3OKJ6Gcy5TRanLiU1uIDT8_9UA/edit?usp=sharing
Introduce the new ephemeral type of topic in multi-topic ingestion. The main difference would be the segment name. For such type of topic, the segment name would be
tableName__topicName__partitionGroupId__sequenceNumber__creationTime
instead oftableName__partitionGroupId__sequenceNumber__creationTime
.This change is supposed to be no-op at the moment because such naming would not be applied to any ingestions. Afterwards, this would be used to:
Other changes:
E.g. when we have streamConfigMaps like
With
"realtime.segment.isBackfillTopic": "true",
in one of the topic, then the segment name would be (assuming 2 partitions per topic)Right now, we are not able to remove
topic1
becausetopic2
's corresponding PinotPartitionId would become 10000 -> 0, 10001- > 1, and inherit topic1's segment commit info.But for this new type of topic, it is safe to remove
topic1_backfill
because it would not mess uptopic1
andtopic2
's PinotPartitionId mapping.