Skip to content

Conversation

@mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Jan 30, 2022

Fixes #14046

Motivation

Fixes AsyncResponsedoes not resume when redirected.

Modifications

  • Add AsyncResponse#resume when redirected.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

Check the box below or label this PR directly (if you have committer privilege).

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 30, 2022
@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

1 similar comment
@mattisonchao
Copy link
Member Author

/pulsarbot rerun-failure-checks

@mattisonchao
Copy link
Member Author

@merlimat @lhotari @Jason918 PTAL :-)

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Change looks good, but would you be able to describe what kind of issues this caused? What was the end user impact? Any sample error logs? That could help find out if a particular problems is fixed by this change.

@mattisonchao
Copy link
Member Author

mattisonchao commented Feb 5, 2022

Change looks good, but would you be able to describe what kind of issues this caused? What was the end user impact? Any sample error logs? That could help find out if a particular problems is fixed by this change.

@lhotari You can see #14046. this issue is because we lost an async response. that will hang the request until timeout when the request needs to redirect to another broker.

@lhotari
Copy link
Member

lhotari commented Feb 7, 2022

@lhotari You can see #14046. this issue is because we lost an async response. that will hang the request until timeout when the request needs to redirect to another broker.

I'm sorry I missed that detail. I wonder if there are other problems that this PR fixes?
The fix is to production code.

@lhotari
Copy link
Member

lhotari commented Feb 7, 2022

Would it be possible to add more test coverage that the missing redirect would be caught by a unit test or similar?

@mattisonchao
Copy link
Member Author

Would it be possible to add more test coverage that the missing redirect would be caught by a unit test or similar?

Sure, i will do it ASAP.

@michaeljmarshall
Copy link
Member

@lhotari - this bug appears to be introduced in #13805, which has not yet made it into a release.

@lhotari lhotari merged commit 90da187 into apache:master Feb 8, 2022
@mattisonchao
Copy link
Member Author

I think if we add a test for the admin side to test topic not own this broker will redirect to another broker action. maybe we need to add this test for all admin APIs that need to validate topic ownership, So, I think it needs a lot of work. ant I think we need to do it in another PR.

BewareMyPower pushed a commit to streamnative/kop that referenced this pull request Feb 9, 2022
…Topic (#1057)

Fixes: #1056

### Motivation

The `testMultiBrokerProduceAndConsumeOnePartitionedTopic` test method fails sporadically. Because the sometimes delete topic will timeout. link PR: apache/pulsar#14060

### Modifications

* remove the call of `deletePartitionedTopic`
BewareMyPower pushed a commit to streamnative/kop that referenced this pull request Feb 9, 2022
…Topic (#1057)

Fixes: #1056

### Motivation

The `testMultiBrokerProduceAndConsumeOnePartitionedTopic` test method fails sporadically. Because the sometimes delete topic will timeout. link PR: apache/pulsar#14060

### Modifications

* remove the call of `deletePartitionedTopic`

(cherry picked from commit 8c7afcd)
BewareMyPower pushed a commit to streamnative/kop that referenced this pull request Feb 9, 2022
…Topic (#1057)

Fixes: #1056

### Motivation

The `testMultiBrokerProduceAndConsumeOnePartitionedTopic` test method fails sporadically. Because the sometimes delete topic will timeout. link PR: apache/pulsar#14060

### Modifications

* remove the call of `deletePartitionedTopic`

(cherry picked from commit 8c7afcd)
BewareMyPower pushed a commit to streamnative/kop that referenced this pull request Feb 9, 2022
…Topic (#1057)

Fixes: #1056

### Motivation

The `testMultiBrokerProduceAndConsumeOnePartitionedTopic` test method fails sporadically. Because the sometimes delete topic will timeout. link PR: apache/pulsar#14060

### Modifications

* remove the call of `deletePartitionedTopic`

(cherry picked from commit 8c7afcd)
eolivelli pushed a commit to datastax/starlight-for-kafka that referenced this pull request Feb 9, 2022
…Topic (#1057)

Fixes: #1056

### Motivation

The `testMultiBrokerProduceAndConsumeOnePartitionedTopic` test method fails sporadically. Because the sometimes delete topic will timeout. link PR: apache/pulsar#14060

### Modifications

* remove the call of `deletePartitionedTopic`

(cherry picked from commit 8c7afcd)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: BrokerServiceLookupTest.testPartitionTopicLookup

5 participants