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

feat: unload partitions when not publishing for better performance #1286

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

Conversation

dpcollins-google
Copy link
Collaborator

No description provided.

@dpcollins-google dpcollins-google requested a review from a team as a code owner December 8, 2022 14:41
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Dec 8, 2022
@product-auto-label product-auto-label bot added the api: pubsublite Issues related to the googleapis/java-pubsublite API. label Dec 8, 2022
@gcf-owl-bot gcf-owl-bot bot requested a review from a team as a code owner December 8, 2022 14:43
@dpcollins-google dpcollins-google force-pushed the publisher-stop-partitions branch from e7b4fd3 to d2bdb77 Compare December 8, 2022 15:32
}
},
SystemExecutors.getFuturesExecutor());
publisher.get().startAsync().awaitRunning();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it ok to block the publish() call here, while the stream is connecting?

In Go, I allowed the wire publisher to accept messages while it is starting up. The messages just get queued until the stream is connected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this issue already exists- so I'm okay with blocking here. We should as a refactor fix this eventually.


private synchronized void onUnloadAlarm() {
if (publisher.isPresent() && !sawPublish) {
publisher.get().stopAsync().awaitTerminated();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to the above, waiting here may block publish() due to the lock held.

@dpcollins-google dpcollins-google force-pushed the publisher-stop-partitions branch from 5bf88a3 to 7921de9 Compare February 3, 2023 15:58
@dpcollins-google dpcollins-google added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsublite Issues related to the googleapis/java-pubsublite API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants