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

Initialize lookup-request variables locally before request-instance recycle #117

Merged
merged 1 commit into from
Nov 17, 2016

Conversation

rdhabalia
Copy link
Contributor

Motivation

As lookup (PartitionMetadata/Lookup-topic) requests being processed async: Lookup-request instance gets recycled immediately after main-io thread completes its process and triggers async-processing of request in a different thread. So, we need to copy request-variables locally for processing-thread before instance gets recycled.

Modifications

Initialize lookup-request variable locally before Lookup-request instance gets recycled.

Result

It returns lookup-response with correct response-value.

@rdhabalia rdhabalia added the type/bug The PR fixed a bug or issue reported a bug label Nov 17, 2016
@rdhabalia rdhabalia added this to the 1.16 milestone Nov 17, 2016
@rdhabalia rdhabalia self-assigned this Nov 17, 2016
@yahoocla
Copy link

CLA is valid!

2 similar comments
@yahoocla
Copy link

CLA is valid!

@yahoocla
Copy link

CLA is valid!

@@ -151,39 +151,42 @@ protected void handleLookup(CommandLookupTopic lookup) {
if (log.isDebugEnabled()) {
log.debug("Received Lookup from {}", remoteAddress);
}
lookupDestinationAsync(getBrokerService().pulsar(), DestinationName.get(lookup.getTopic()),
final long requestId = lookup.getRequestId();
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting is off here

@@ -238,17 +241,18 @@ protected void handleSubscribe(final CommandSubscribe subscribe) {
} else {
authorizationFuture = CompletableFuture.completedFuture(true);
}
final String topicName = subscribe.getTopic();
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting

@@ -71,7 +71,7 @@

private final AtomicLong producerIdGenerator = new AtomicLong();
private final AtomicLong consumerIdGenerator = new AtomicLong();
protected static final AtomicLong requestIdGenerator = new AtomicLong();
protected final AtomicLong requestIdGenerator = new AtomicLong();
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be private

@@ -364,6 +364,10 @@ long newConsumerId() {
long newRequestId() {
return requestIdGenerator.getAndIncrement();
}

ConnectionPool getCnxPool() {
return cnxPool;
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting

@rdhabalia
Copy link
Contributor Author

Addressed the changes.

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.

👍

@merlimat merlimat merged commit 70ae671 into apache:master Nov 17, 2016
@rdhabalia rdhabalia deleted the lk_rcy branch January 23, 2017 22:08
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
* fixing response codes and cli

* cleaning up

* fixing unit tests
hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020
massakam pushed a commit to massakam/pulsar that referenced this pull request Feb 9, 2021
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
dragonls pushed a commit to dragonls/pulsar that referenced this pull request Oct 21, 2022
dragonls pushed a commit to dragonls/pulsar that referenced this pull request Oct 21, 2022
…est !56)

Fix the system topic schema not compatible bug. (apache#117)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants