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

[pulsar-broker] topic resources use metadata-store api #9485

Merged
merged 2 commits into from
Feb 18, 2021

Conversation

rdhabalia
Copy link
Contributor

Motivation

This PR is on top of #9351, with this change, topuc-resources will start using metadata-api and remove zk-dependency.

@rdhabalia rdhabalia added this to the 2.8.0 milestone Feb 4, 2021
@rdhabalia rdhabalia self-assigned this Feb 4, 2021
@rdhabalia rdhabalia force-pushed the zk_rest_topic branch 5 times, most recently from 7040951 to be4b114 Compare February 6, 2021 02:04
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM. Should this get rebased on master?

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

I really like how much we are making core Pulsar code much more simpler!

Overall it looks good to me, I left some minor comments

}
});
} else if (ex.getCause() instanceof BadVersionException) {
log.warn("[{}] Fail to create topic partition {} with concurrent modification, retry now.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this message still valid ?
It looks like we are completing the operation with success

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good catch. it's related to #9501 , I will fix log.

log.error("Failed to delete partitioned topic.", e);
asyncResponse.resume(new RestException(e));
try {
namespaceResources().getPartitionedTopicResouces().deleteAsync(path).thenAccept(r2 -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a typo?
getPartitionedTopicResouces -> getPartitionedTopicResources

@Override
public void setup() throws Exception {
super.internalSetup();
}

@AfterClass
@AfterMethod
Copy link
Contributor

Choose a reason for hiding this comment

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

don't forget "alwaysRun = true", otherwise resources won't be disposed in case of test failure

try {
bookie.shutdown();
} catch (Exception e) {
LOG.warn("failed to shutdown bookie", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

in case of InterruptedException we should set Thread.currentThead().interrupt();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't throw InterruptedException but in some cases it was throwing other runtime exception in test, while closing the resources. So, it helps test to clean other resources by handling this failure.

[pulsar-broker] MockZK: Handle zk-children watch notification

fix test

fix sync function initialization

fix tests

fix zk-create

add timeout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Project-10: Replace Zookeeper API access in REST resources with Pluggable metadata resources
3 participants