Skip to content

Rename IMSC.announceHistoricalSegments to commitSegments#15021

Merged
kfaraz merged 1 commit intoapache:masterfrom
kfaraz:prepare_for_concurrent
Sep 21, 2023
Merged

Rename IMSC.announceHistoricalSegments to commitSegments#15021
kfaraz merged 1 commit intoapache:masterfrom
kfaraz:prepare_for_concurrent

Conversation

@kfaraz
Copy link
Copy Markdown
Contributor

@kfaraz kfaraz commented Sep 21, 2023

This PR pulls out some changes from #14407 to simplify that PR

Changes

  • Rename IndexerMetadataStorageCoordinator.announceHistoricalSegments to commitSegments
  • Rename the overloaded method to commitSegmentsAndMetadata
  • Fix some typos

newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
);
final Set<DataSegment> announced = getMetadataStorageCoordinator().announceHistoricalSegments(segments);
final Set<DataSegment> announced = getMetadataStorageCoordinator().commitSegments(segments);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It might be nice to rename announced to committed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure 👍🏻
If CI fails for any reason, I will include this change. Otherwise, I will include it in the other PR.

Copy link
Copy Markdown
Contributor

@AmatyaAvadhanula AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

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

LGTM except for a minor suggestion. +1 after all checks have passed

@kfaraz
Copy link
Copy Markdown
Contributor Author

kfaraz commented Sep 21, 2023

Merging as IT failure is unrelated.

@kfaraz kfaraz merged commit 409bffe into apache:master Sep 21, 2023
@kfaraz kfaraz deleted the prepare_for_concurrent branch September 21, 2023 10:49
@LakshSingla LakshSingla added this to the 28.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants