-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add support minor compaction with segment locking #7547
Conversation
@jihoonson these measurements are great, thanks! I will try to finish up my review sometime this week 😅 |
|
||
/** | ||
* Abstract class for batch tasks like {@link IndexTask}. | ||
* Provides some methods ({@link #determineSegmentGranularity} and {@link #determineSegmentGranularity}) for easily acquiring task |
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.
determineSegmentGranularity
is mentioned twice here
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.
Thanks, fixed javadoc.
{ | ||
if (requireLockExistingSegments()) { | ||
if (isPerfectRollup()) { | ||
log.info("Using timeChunk lock for perfrect rollup"); |
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.
perfrect -> perfect
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.
Thanks, fixed.
} | ||
|
||
@Nullable | ||
public OverwritingRootGenerationPartitions getOverwritingSegmentMeta(Interval interval) | ||
private static class LockGranularityDeterminResult |
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.
LockGranularityDeterminResult -> LockGranularityDetermineResult
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.
Thanks, fixed.
|
||
for (SegmentIdWithShardSpec segmentIdentifier : idsPerInterval) { | ||
shardSpecMap.computeIfAbsent(interval, k -> new ArrayList<>()).add(segmentIdentifier.getShardSpec()); | ||
// The shardSpecs for partitinoing and publishing can be different if isExtendableShardSpecs = true. |
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.
partitinoing -> partitioning
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.
Thanks, fixed.
if (bucketIntervals.isPresent()) { | ||
// If the granularity spec has explicit intervals, we just need to find the interval (of the segment | ||
// granularity); we already tried to lock it at task startup. | ||
// If granularity spec has explicit intervals, we just need to find the version accociated to the interval. |
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.
accociated -> associated
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.
Thanks, fixed.
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
/** | ||
* Returns true if this segment overshadows the other segment. | ||
*/ | ||
default boolean isOvershadow(T other) |
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 think the purpose of this method would be clearer if it was called something like overshadows
or doesOvershadow
or willOvershadow
. It reads with the intention x.overshadows(y)
of x.willOvershadow(y)
is very clear that it's a check if y will be overshadowed by x.
I think isOvershadow
it too close to x is overshadowed by y
, which is why I keep having to check the javadoc to remember which one is the overshadowed segment, and will also help it standout/distinguish it from the timeline isOvershadowed
method.
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.
Renamed to overshadows()
.
final Set<DataSegment> inputSegments = isUseSegmentLock() | ||
? getSegmentLockHelper().getLockedExistingSegments() | ||
: null; | ||
final SegmentsAndMetadata published = awaitPublish(driver.publishAll(inputSegments, publisher), pushTimeout); |
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.
is driver.publishAll
going to be sad if inputSegments
is null because not using segment lock? could you explain this a bit better in comments?
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.
Added a comment.
|
||
private boolean tryTimeChunkLock(TaskActionClient client, List<Interval> intervals) throws IOException | ||
{ | ||
// In this case, the intervals to lock must be alighed with segmentGranularity if it's defined |
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.
typo:
... must be aligned with ...
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.
Thanks, fixed.
} | ||
|
||
final List<Short2ObjectMap.Entry<AtomicUpdateGroup<T>>> found = new ArrayList<>(); | ||
while (current != null && rangeOfAug.contains(current.getKey())) { |
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 method could maybe use a few more comments to make understanding what is going on a bit easier
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.
👍 added comments and javadoc.
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, 👍
@jon-wei @clintropolis thank you for the review!! |
before the version,we use hadoop-task and kafka-task ingestion data into the same datasource, is success. now we must run compact task to solve the exception Is there any new solution to this problem? |
Apologize for the huge PR. I tried to split into sub PRs, but I think it's not easy split more because new changes for segment locking are tightly coupled together and it won't be easy to understand if they are in separate PRs.
This PR fixes #7491 and now it's ready for review.
Here are the key classes to be reviewed.
Overshadowable
New interface to represent a class which can have overshadow relation between its instances. Ex) dataSegment
ShardSpecFactory
New interface to be used to allocate segments remotely in the overlord.
NumberedOverwriteShardSpec
New shardSpec for segments which may overshadow others with their minor version.
DataSegment
getVersion
is renamed togetMajorVersion
.VersionedIntervalTimeline
Improved to consider the new overshadow relation properly.
OvershadowableManager
A new class manages the state of AtomicUpdateGroup.
AtomicUpdateGruop
A set of PartitionChunks which should be atomically visible or not in the timeline.
SegmentTransactionalInsertAction
Added a new parameter
segmentsToBeOverwritten
. Shouldn't be empty if the new segments are created with segment locks.AbstractBatchIndexTask
Abstract class for batch tasks like indexTask or parallelIndexTask. Provides some methods for easily acquiring task locks.
IndexTaskSegmentAllocator
Segment allocator interface for only IndexTask. It has 3 different modes for allocating segments.
The new segment locking is currently available for only Kafka/Kinesis index task, index task, and parallel index task.
This PR is marked as
Incompatible
because it introduces a new shardSpec. It's not possible to roll back after this PR. For rolling update while kafka/kinesis indexing service is running, any kind of overwriting tasks (including compaction task and index task) should be stopped first. Otherwise, kafka/kinesis index tasks might fail when they list segments with the new shardSpec generated by overwriting tasks. Native parallel index tasks cannot be run in middleManagers of mixed versions, so they should be stopped first before update.Currently there is a known issue of potential race in task locking. If two or more overwriting tasks try to acquire segment locks for overlapped sets of segments, each task acquires locks one by one per segment in the current implementation. This may lead to failures of both tasks if they hold locks necessary for each other. However, I think this happens rarely and so will fix in a follow-up PR.
I've been testing this PR in our in-house cluster since I raised this PR and it looks fine so far.