Fix getting partition metadata of a nonexistent topic returns 0 #10601

BewareMyPower · 2021-05-16T14:52:33Z

Motivation

Actually #8818 didn't fix the issue. The reason is when PulsarAdmin tries to get partition metadata, the checkAllowAutoCreation query param is false, so the topic existence check will never be performed.

Modifications

Check if the topic exists when the partition's metadata is 0 and checkAllowAutoCreation is false. See comments for detail explanations.
Change the current tests to make sure the check works.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as AdminApiTest#partitionedTopics.

BewareMyPower · 2021-05-16T15:39:31Z

Mark it as WIP first because there may be still some failed tests.

BewareMyPower · 2021-05-17T01:38:36Z

It looks like there're still two flaky tests that can always pass in my local env.

LoadBalancerTest.testBrokerRanking
MessagePublishBufferThrottleTest.testBlockByPublishRateLimiting

BewareMyPower · 2021-05-17T02:27:47Z

/pulsarbot run-failure-checks

eolivelli

awesome work.
can you please also verify the behaviour for non-persistent topics ?
it happens quite often that we make a fix for persistent topics but we let non-persistent topics behave in a inconsistent way

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest.java

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v1/V1_AdminApiTest.java

...roker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceAutoTopicCreationTest.java

BewareMyPower · 2021-05-17T11:07:24Z

Thanks @eolivelli for your advice. I tried to add tests for non-persistent topics and found some difference between persistent topics and non-persistent topics. In this PR, I'll only fix the getPartitionedTopicMetadata implementation of non-persistent topics and mark the difference as comments.

eolivelli · 2021-05-17T11:27:12Z

@BewareMyPower can you please follow up with a new PR for non persistent topics ?
I am fine with merging this patch (but please check my comments)

BewareMyPower · 2021-05-17T11:36:37Z

@eolivelli I'll push a commit to fix the getPartitionedTopicMetadata for non-persistent topics in this PR soon. The other problems are subscription related admin operations. I may not have time to fix them recently, maybe we can open an issue to track it first.

…d tests

BewareMyPower · 2021-05-17T16:09:25Z

@codelipenghui Please include this fix in 2.8.0 if it's merged

BewareMyPower · 2021-05-18T08:21:26Z

@eolivelli PTAL again

eolivelli

great work

Upgrade pulsar dependency to a newer version (2.8.0-rc-202105182205) that contains apache/pulsar#10601, which corrected the wrong semantics of `getPartitionedTopicMetadataAsync` that returns 0 for a non-existed topic.

…he#10601)

…11557) Fixes #11551 ### Motivation Currently there're some bugs of C++ client and some tests cannot pass: 1. Introduced from #10601 because it changed the behavior of the admin API to get partition metadata while the C++ implementation relies on the original behavior to create topics automatically. So any test that uses HTTP lookup will fail. - AuthPluginTest.testTlsDetectHttps - AuthPluginToken.testTokenWithHttpUrl - BasicEndToEndTest.testHandlerReconnectionLogic - BasicEndToEndTest.testV2TopicHttp - ClientDeduplicationTest.testProducerDeduplication 2. Introduced from #11029 and #11486 , the implementation will iterate more than once even there's only one valid resolved IP address. - ClientTest.testConnectTimeout In addition, there's an existed flaky test from very early time: ClientTest.testLookupThrottling. Python tests are also broken. Because it must run after all C++ tests passed, they're also not exposed. 1. Some tests in `pulsar_test.py` might encounter `Timeout` error when creating producers or consumers. 2. Some tests in `schema_test.py` failed because some comparisons between two `ComplexRecord`s failed. Since the CI test of C++ client would never fail after #10309 (will be fixed by #11575), all PRs about C++ or Python client are not verified even if CI passed. Before #11575 is merged, we need to fix all existed bugs of C++ client. ### Modifications Corresponding to the above tests group, this PR adds following modifications: 1. Add the `?checkAllowAutoCreation=true` URL suffix to allow HTTP lookup to create topics automatically. 2. When iterating through a resolved IP list, increase the iterator first, then run the connection timer and try to connect the next IP. Regarding to the flaky `testLookupThrottling`, this PR adds a `client.close()` at the end of test and fix the `ClientImpl::close` implementation. Before this PR, if there're no producers or consumers in a client, the `close()` method wouldn't call `shutdown()` to close connection poll and executors. Only after the `Client` instance was destructed would the `shutdown()` method be called. In this case, this PR calls `handleClose` instead of invoking callback directly. In addition, change the log level of this test to debug. This PR also fixes the failed timeout Python tests, some are caused by incorrect import of classes, some are caused by `client` was not closed. Regarding to Python schema tests, in Python2, `self.__ne__(other)` is not equivalent to `not self.__eq__(other)` when the default `__eq__` implementation is overwritten. If a `Record` object has a field whose type is also `Record`, the `Record.__ne__` method will be called, see https://github.com/apache/pulsar/blob/ddb5fb0e062c2fe0967efce2a443a31f9cd12c07/pulsar-client-cpp/python/pulsar/schema/definition.py#L138-L139 but it just uses the default implementation to check whether they're not equal. The custom `__eq__` method won't be called. Therefore, this PR implement `Record.__ne__` explicitly to call `Record.__eq__` so that the comparison will work for Python2. ### Verifying this change We can only check the workflow output to verify this change.

…pache#11557) Fixes apache#11551 ### Motivation Currently there're some bugs of C++ client and some tests cannot pass: 1. Introduced from apache#10601 because it changed the behavior of the admin API to get partition metadata while the C++ implementation relies on the original behavior to create topics automatically. So any test that uses HTTP lookup will fail. - AuthPluginTest.testTlsDetectHttps - AuthPluginToken.testTokenWithHttpUrl - BasicEndToEndTest.testHandlerReconnectionLogic - BasicEndToEndTest.testV2TopicHttp - ClientDeduplicationTest.testProducerDeduplication 2. Introduced from apache#11029 and apache#11486 , the implementation will iterate more than once even there's only one valid resolved IP address. - ClientTest.testConnectTimeout In addition, there's an existed flaky test from very early time: ClientTest.testLookupThrottling. Python tests are also broken. Because it must run after all C++ tests passed, they're also not exposed. 1. Some tests in `pulsar_test.py` might encounter `Timeout` error when creating producers or consumers. 2. Some tests in `schema_test.py` failed because some comparisons between two `ComplexRecord`s failed. Since the CI test of C++ client would never fail after apache#10309 (will be fixed by apache#11575), all PRs about C++ or Python client are not verified even if CI passed. Before apache#11575 is merged, we need to fix all existed bugs of C++ client. ### Modifications Corresponding to the above tests group, this PR adds following modifications: 1. Add the `?checkAllowAutoCreation=true` URL suffix to allow HTTP lookup to create topics automatically. 2. When iterating through a resolved IP list, increase the iterator first, then run the connection timer and try to connect the next IP. Regarding to the flaky `testLookupThrottling`, this PR adds a `client.close()` at the end of test and fix the `ClientImpl::close` implementation. Before this PR, if there're no producers or consumers in a client, the `close()` method wouldn't call `shutdown()` to close connection poll and executors. Only after the `Client` instance was destructed would the `shutdown()` method be called. In this case, this PR calls `handleClose` instead of invoking callback directly. In addition, change the log level of this test to debug. This PR also fixes the failed timeout Python tests, some are caused by incorrect import of classes, some are caused by `client` was not closed. Regarding to Python schema tests, in Python2, `self.__ne__(other)` is not equivalent to `not self.__eq__(other)` when the default `__eq__` implementation is overwritten. If a `Record` object has a field whose type is also `Record`, the `Record.__ne__` method will be called, see https://github.com/apache/pulsar/blob/ddb5fb0e062c2fe0967efce2a443a31f9cd12c07/pulsar-client-cpp/python/pulsar/schema/definition.py#L138-L139 but it just uses the default implementation to check whether they're not equal. The custom `__eq__` method won't be called. Therefore, this PR implement `Record.__ne__` explicitly to call `Record.__eq__` so that the comparison will work for Python2. ### Verifying this change We can only check the workflow output to verify this change.

…11557) Fixes #11551 ### Motivation Currently there're some bugs of C++ client and some tests cannot pass: 1. Introduced from #10601 because it changed the behavior of the admin API to get partition metadata while the C++ implementation relies on the original behavior to create topics automatically. So any test that uses HTTP lookup will fail. - AuthPluginTest.testTlsDetectHttps - AuthPluginToken.testTokenWithHttpUrl - BasicEndToEndTest.testHandlerReconnectionLogic - BasicEndToEndTest.testV2TopicHttp - ClientDeduplicationTest.testProducerDeduplication 2. Introduced from #11029 and #11486 , the implementation will iterate more than once even there's only one valid resolved IP address. - ClientTest.testConnectTimeout In addition, there's an existed flaky test from very early time: ClientTest.testLookupThrottling. Python tests are also broken. Because it must run after all C++ tests passed, they're also not exposed. 1. Some tests in `pulsar_test.py` might encounter `Timeout` error when creating producers or consumers. 2. Some tests in `schema_test.py` failed because some comparisons between two `ComplexRecord`s failed. Since the CI test of C++ client would never fail after #10309 (will be fixed by #11575), all PRs about C++ or Python client are not verified even if CI passed. Before #11575 is merged, we need to fix all existed bugs of C++ client. ### Modifications Corresponding to the above tests group, this PR adds following modifications: 1. Add the `?checkAllowAutoCreation=true` URL suffix to allow HTTP lookup to create topics automatically. 2. When iterating through a resolved IP list, increase the iterator first, then run the connection timer and try to connect the next IP. Regarding to the flaky `testLookupThrottling`, this PR adds a `client.close()` at the end of test and fix the `ClientImpl::close` implementation. Before this PR, if there're no producers or consumers in a client, the `close()` method wouldn't call `shutdown()` to close connection poll and executors. Only after the `Client` instance was destructed would the `shutdown()` method be called. In this case, this PR calls `handleClose` instead of invoking callback directly. In addition, change the log level of this test to debug. This PR also fixes the failed timeout Python tests, some are caused by incorrect import of classes, some are caused by `client` was not closed. Regarding to Python schema tests, in Python2, `self.__ne__(other)` is not equivalent to `not self.__eq__(other)` when the default `__eq__` implementation is overwritten. If a `Record` object has a field whose type is also `Record`, the `Record.__ne__` method will be called, see https://github.com/apache/pulsar/blob/ddb5fb0e062c2fe0967efce2a443a31f9cd12c07/pulsar-client-cpp/python/pulsar/schema/definition.py#L138-L139 but it just uses the default implementation to check whether they're not equal. The custom `__eq__` method won't be called. Therefore, this PR implement `Record.__ne__` explicitly to call `Record.__eq__` so that the comparison will work for Python2. ### Verifying this change We can only check the workflow output to verify this change. (cherry picked from commit 4919a82)

…he#10601)

…pache#11557) Fixes apache#11551 ### Motivation Currently there're some bugs of C++ client and some tests cannot pass: 1. Introduced from apache#10601 because it changed the behavior of the admin API to get partition metadata while the C++ implementation relies on the original behavior to create topics automatically. So any test that uses HTTP lookup will fail. - AuthPluginTest.testTlsDetectHttps - AuthPluginToken.testTokenWithHttpUrl - BasicEndToEndTest.testHandlerReconnectionLogic - BasicEndToEndTest.testV2TopicHttp - ClientDeduplicationTest.testProducerDeduplication 2. Introduced from apache#11029 and apache#11486 , the implementation will iterate more than once even there's only one valid resolved IP address. - ClientTest.testConnectTimeout In addition, there's an existed flaky test from very early time: ClientTest.testLookupThrottling. Python tests are also broken. Because it must run after all C++ tests passed, they're also not exposed. 1. Some tests in `pulsar_test.py` might encounter `Timeout` error when creating producers or consumers. 2. Some tests in `schema_test.py` failed because some comparisons between two `ComplexRecord`s failed. Since the CI test of C++ client would never fail after apache#10309 (will be fixed by apache#11575), all PRs about C++ or Python client are not verified even if CI passed. Before apache#11575 is merged, we need to fix all existed bugs of C++ client. ### Modifications Corresponding to the above tests group, this PR adds following modifications: 1. Add the `?checkAllowAutoCreation=true` URL suffix to allow HTTP lookup to create topics automatically. 2. When iterating through a resolved IP list, increase the iterator first, then run the connection timer and try to connect the next IP. Regarding to the flaky `testLookupThrottling`, this PR adds a `client.close()` at the end of test and fix the `ClientImpl::close` implementation. Before this PR, if there're no producers or consumers in a client, the `close()` method wouldn't call `shutdown()` to close connection poll and executors. Only after the `Client` instance was destructed would the `shutdown()` method be called. In this case, this PR calls `handleClose` instead of invoking callback directly. In addition, change the log level of this test to debug. This PR also fixes the failed timeout Python tests, some are caused by incorrect import of classes, some are caused by `client` was not closed. Regarding to Python schema tests, in Python2, `self.__ne__(other)` is not equivalent to `not self.__eq__(other)` when the default `__eq__` implementation is overwritten. If a `Record` object has a field whose type is also `Record`, the `Record.__ne__` method will be called, see https://github.com/apache/pulsar/blob/ddb5fb0e062c2fe0967efce2a443a31f9cd12c07/pulsar-client-cpp/python/pulsar/schema/definition.py#L138-L139 but it just uses the default implementation to check whether they're not equal. The custom `__eq__` method won't be called. Therefore, this PR implement `Record.__ne__` explicitly to call `Record.__eq__` so that the comparison will work for Python2. ### Verifying this change We can only check the workflow output to verify this change.

rdhabalia · 2022-11-16T18:08:14Z

This PR breaks all previous clients who would like to use HTTP lookup. HTTP lookup at client side expects partitioned metadata with 0 partition and this PR gives TopicNotFoundException to client and old client couldn't handle it and won't be able to create a new topic which breaks topic creation semantics of Pulsar.
I have seen multiple such instances in past where we broke the compatibility and it's not acceptable.

eolivelli · 2022-11-17T10:11:30Z

@rdhabalia
I agree that this is a behaviour change in the API (although we introduced it in a major release, 2.8.0)

I think that this is fixing a bug, if the topic does not exist we should return "not found".

I understand that this is painful for clients who expected "0" instead of "Not Found", we should have advertised this change better or dealt with it in a different way.

Actually at the time of 2.8.0 we didn't have good tools/processes to deal with this kind of breaking changes (generally speaking breaking changes shouldn't be allowed at all!) .

Now the community evolved and before doing this kind of change we have to follow a more strict process (PIP) and that's should allow us to not fall anymore in this kind of problems for existing users.

rdhabalia · 2022-11-17T18:34:08Z

I am going to revert this change as old clients are just failing. let me know if anyone has any objections. I have experienced such intentional changes in past as well and I would like to check if anyone thinks that it's fine to do destructive changes just because their PRs will be merged then it's not a good practice. So, this time I want to set the message clear for these intentionally breaking changes.

eolivelli · 2022-11-17T19:11:26Z

This change has been released with 2.8 that now is a very old release (very close to be considered obsolete). I am aware of many new users since 2
8 and for them we will introduce another breaking change.

I think that you could send out PR and start a discussion on dev@

rdhabalia · 2022-11-18T00:31:46Z

before this change, the previous behavior was there for 8 years so, I definitely reject the claim that it's been there for the last 2 years. And again: this change is breaking the lookup for an old clients. it's a P0 and it's not acceptable with any justification.

rdhabalia · 2022-11-18T00:37:20Z

also I don't understand why do we defend breaking changes.? what's the main motivation here? does anyone want me to find out all past breaking change list and we have to live with those changes by patching on top of open source?

BewareMyPower · 2022-11-18T03:35:54Z

This PR was included in 2.8.0 and didn't break the compatibility with 2.7.x because #4963 introduced a query param checkAllowAutoCreation in HTTP client. Actually #4963 introduced a breaking change (allowAutoTopicCreationType=partitioned) so that the query param was added. The breaking change has been reverted in #5308 (allowAutoTopicCreationType=non-partitioned), but the client side changes are not reverted.

The C++ client tests should expose the breaking change because there was no checkAllowAutoCreation in C++ client before #11557. Unfortunately, the CI in master was accidentally broken at that time (see #11575) so the C++ client tests passed.

I agree that it's a breaking change. But this new behavior is also needed in some cases to differ a non-partitioned topic and a non-existent topic.

I prefer to add a new config to keep the compatibility for old version clients. Reverting this PR would also bring many breaking changes for those external systems that depend on Pulsar 2.8.0 or later. @rdhabalia @eolivelli

I will send an mail to dev and reference the discussion here soon.

BewareMyPower · 2022-11-18T04:03:32Z

Let's continue the discussion here: https://lists.apache.org/thread/88t1xxf68j092k09srdwyzj1tk4ml5n9

…of a nonexistent topic ### Motivation apache#10601 (comment) apache#10601 changes the behavior when querying partitions of a topic that is not created. Before apache#10601, 0 is returned. After apache#10601, an exception will be thrown to indicate the topic does not exist. It leads to the incompatibility with some old Pulsar clients that do not add the "checkAllowAutoCreation=true" query param. If they use HTTP service URL like "http://localhost:8080", when accessing a topic that does not exist, the client will fail. The affected Pulsar clients include Java client <= 2.4.2 and C++/Python client <= 2.8.0. ### Modifications Add an option `checkTopicExistsWhenQueryPartitions` (default: true) to determine the behavior. Disable this option to keep the original behavior that 0 will be returned when querying partitions of a nonexistent topic.

Fix getting partition metadata of a nonexistent topic returns 0

7406f53

BewareMyPower changed the title ~~Fix getting partition metadata of a nonexistent topic returns 0~~ [WIP] Fix getting partition metadata of a nonexistent topic returns 0 May 16, 2021

Fix testGetPartitionedMetadataWithoutCheckAllowAutoCreation

0134d90

BewareMyPower changed the title ~~[WIP] Fix getting partition metadata of a nonexistent topic returns 0~~ Fix getting partition metadata of a nonexistent topic returns 0 May 17, 2021

Fix V1_AdminApiTest

32b6078

BewareMyPower self-assigned this May 17, 2021

BewareMyPower requested review from 315157973, codelipenghui, eolivelli, hangc0276, jiazhai, lhotari and merlimat and removed request for codelipenghui and eolivelli May 17, 2021 09:06

eolivelli requested changes May 17, 2021

View reviewed changes

Fix getPartitionedTopicMetadata fail for non-persistent topics and ad…

b94dcf5

…d tests

BewareMyPower mentioned this pull request May 17, 2021

Topic admin operations doesn't work well for non-persistent partitioned topics #10615

Open

sijie mentioned this pull request May 17, 2021

ISSUE-10615: Topic admin operations doesn't work well for non-persistent partitioned topics streamnative/pulsar-archived#2533

Open

sijie approved these changes May 18, 2021

View reviewed changes

eolivelli approved these changes May 18, 2021

View reviewed changes

eolivelli merged commit 403b57a into apache:master May 18, 2021

BewareMyPower deleted the bewaremypower/fix-get-partition-metadata branch May 18, 2021 09:32

BewareMyPower mentioned this pull request May 20, 2021

Apply getPartitionedTopicMetadata API with correct semantics streamnative/kop#522

Merged

freeznet mentioned this pull request Jun 22, 2021

regenerate the certs to work with Pulsar 2.8.0 and Java 11 apache/pulsar-client-go#548

Merged

1 task

yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021

Fix getting partition metadata of a nonexistent topic returns 0 (apac…

8fa7eb0

…he#10601)

codelipenghui modified the milestones: 2.9.0, 2.8.0 Jul 1, 2021

eladar2000 mentioned this pull request Jul 1, 2021

[BUG] Stream failure if the topic doesn't exist with Pulsar 2.8.0 streamnative/pulsar-flink#366

Closed

BewareMyPower mentioned this pull request Aug 4, 2021

[C++/Python] Fix bugs that were not exposed by broken C++ CI before #11557

Merged

BewareMyPower mentioned this pull request Oct 9, 2021

fix offset data loss streamnative/kop#799

Merged

bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022

Fix getting partition metadata of a nonexistent topic returns 0 (apac…

a36a190

…he#10601)

This was referenced Nov 24, 2022

[improve][broker] Add an option to return 0 when querying partitions of a nonexistent topic BewareMyPower/pulsar#10

Open

[improve][broker] Add an option to return 0 when querying partitions of a nonexistent topic #18594

Open

rdhabalia mentioned this pull request Nov 30, 2022

[improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url #18663

Merged

13 tasks

Fix getting partition metadata of a nonexistent topic returns 0 #10601

Fix getting partition metadata of a nonexistent topic returns 0 #10601

Uh oh!

Conversation

BewareMyPower commented May 16, 2021

Motivation

Modifications

Verifying this change

Uh oh!

BewareMyPower commented May 16, 2021

Uh oh!

BewareMyPower commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BewareMyPower commented May 17, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BewareMyPower commented May 17, 2021

Uh oh!

eolivelli commented May 17, 2021

Uh oh!

BewareMyPower commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BewareMyPower commented May 17, 2021

Uh oh!

BewareMyPower commented May 18, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

rdhabalia commented Nov 16, 2022

Uh oh!

eolivelli commented Nov 17, 2022

Uh oh!

rdhabalia commented Nov 17, 2022

Uh oh!

eolivelli commented Nov 17, 2022

Uh oh!

rdhabalia commented Nov 18, 2022

Uh oh!

rdhabalia commented Nov 18, 2022

Uh oh!

BewareMyPower commented Nov 18, 2022

Uh oh!

BewareMyPower commented Nov 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BewareMyPower commented May 17, 2021 •

edited

Loading

BewareMyPower commented May 17, 2021 •

edited

Loading