Skip to content
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

Remove unneeded metadata read during update event generation #11829

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

grantatspothero
Copy link
Contributor

Followup from this PR:
#10523

The above PR removed unnecessary objectstore reads after commit, but there was 1 I missed. SnapshotProducer.notifyListeners has the same problem of reading metadata from objectstore instead of just reading the in memory committed Snapshot object.

@github-actions github-actions bot added the core label Dec 19, 2024
@grantatspothero grantatspothero force-pushed the gn/removeUnneededMetadataReadUpdateEvent branch 2 times, most recently from ad81312 to 28984fe Compare December 19, 2024 21:28
@@ -475,10 +475,14 @@ public void commit() {
}
}

Object updateEvent(Snapshot committedSnapshot) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PendingUpdate.updateEvent only usage is in SnapshotProducer currently so could change the interface directly to updateEvent(Snapshot committedSnapshot), but did not want to make a backwards incompatible API change

The whole update event/listener functionality seems untouched for years.

@grantatspothero grantatspothero force-pushed the gn/removeUnneededMetadataReadUpdateEvent branch from 28984fe to a710e1c Compare December 20, 2024 18:40
@grantatspothero grantatspothero force-pushed the gn/removeUnneededMetadataReadUpdateEvent branch from a710e1c to 072bacf Compare December 20, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant