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

[fix][broker] Set the ServiceUnitStateChannel topic compaction threshold explicitly and other bug fixes #22048

Closed

Conversation

heesung-sn
Copy link
Contributor

@heesung-sn heesung-sn commented Feb 12, 2024

Motivation

We better set the compaction threshold of the serviceUnitStateChannel topic when initializing itself instead of relying on other components' init.

Also, I found the lookup and unload request often timeout by

Modifications

  • Set the compaction threshold of the serviceUnitStateChannel topic in the monitor thread
  • return timeout completed future from deferGetOwnerRequest
  • Release metadataAndPayload buffer in RawBatchMessageContainerImpl.
  • Automatically restart load data store producer or table view if inactive.
  • Updated getActiveOwnerAsync logic for better synchronization of ownership checks and deferred lookup handling
  • added minor retry logic in the ExtensibleLoadManager tests

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/heesung-sn/pulsar/pull/61/files

@heesung-sn heesung-sn requested a review from lhotari February 12, 2024 23:42
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 12, 2024
@heesung-sn heesung-sn changed the title [improve][broker] Set the serviceUnitStateChannel topic compaction threshold explicitly [improve][broker] Set the ServiceUnitStateChannel topic compaction threshold explicitly Feb 12, 2024
Comment on lines 987 to 988
admin.topicPolicies()
.setCompactionThreshold(ServiceUnitStateChannelImpl.TOPIC, threshold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put this in a finally block to limit the blast radius of this test if it fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this should always pass. I wonder in what cases this setCompactionThreshold might fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry for the confusion. It's not that setCompactionThreshold might fail, but anything in between the previous call to this "reset" call might (i.e., lines https://github.com/apache/pulsar/pull/22048/files#diff-0bb4e0f521ef681668abbb30387c7de6ebf6cfce7dcbdd3d2f318ac50f6cc8abR967-R986). So put the entire respective block in a try and this in a finally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see. nice catch. updated.

@@ -298,6 +299,14 @@ public synchronized void start() throws PulsarServerException {

ExtensibleLoadManagerImpl.createSystemTopic(pulsar, TOPIC);

if (config.isSystemTopicAndTopicLevelPoliciesEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this flag returns false? Is that a valid way to configure Pulsar with this code? would it make sense to prevent such configuration in the first place, or at least log a warning?

@heesung-sn heesung-sn marked this pull request as draft February 14, 2024 23:59
@heesung-sn heesung-sn force-pushed the pip-192-fix-compaction-threshold branch 3 times, most recently from a5421e7 to 56ae3e9 Compare February 15, 2024 00:54
@heesung-sn heesung-sn changed the title [improve][broker] Set the ServiceUnitStateChannel topic compaction threshold explicitly [fix][broker] Set the ServiceUnitStateChannel topic compaction threshold explicitly and other bug fixes Feb 15, 2024
@heesung-sn heesung-sn force-pushed the pip-192-fix-compaction-threshold branch from 56ae3e9 to 0942c13 Compare February 15, 2024 01:41
@heesung-sn heesung-sn force-pushed the pip-192-fix-compaction-threshold branch from 0942c13 to ec93862 Compare February 15, 2024 01:41
@heesung-sn heesung-sn closed this Feb 15, 2024
@heesung-sn
Copy link
Contributor Author

re-raised the pr here. #22064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants