[C++][Python] Add connection timeout configuration #11029

BewareMyPower · 2021-06-23T04:11:15Z

Fixes #10747

Motivation

This PR is a catchup of #2852 and adds connection timeout configuration to C++ and Python client.

Modifications

Add a PeriodicTask class to execute tasks periodically and the relate unit tests: PeriodicTastTest.
Use PeriodicTask to register a timer before connecting to broker asynchronously, if the connection was not established when the timer is triggered, close the socket so that handleTcpConnected can be triggered immediately with a failure.
Add connection timeout (in milliseconds) to both C++ and Python clients.
Add ClientTest.testConnectTimeout (C++) and test_connect_timeout (Python) and to verify the connection timeout works.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

PeriodicTaskTest
ClientTest.testConnectTimeout
test_connect_timeout

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

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

BewareMyPower · 2021-06-23T06:09:27Z

/pulsarbot run-failure-checks

BewareMyPower · 2021-06-23T14:19:26Z

It looks like the Python Functions became broken after this PR, I'm going to fixing it soon.

freeznet

@BewareMyPower I have left one comment that fixes function CI, PTAL.

pulsar-client-cpp/python/pulsar/__init__.py

Fixes #10747 ### Motivation This PR is a catchup of #2852 and adds connection timeout configuration to C++ and Python client. ### Modifications - Add a `PeriodicTask` class to execute tasks periodically and the relate unit tests: `PeriodicTastTest`. - Use `PeriodicTask` to register a timer before connecting to broker asynchronously, if the connection was not established when the timer is triggered, close the socket so that `handleTcpConnected` can be triggered immediately with a failure. - Add connection timeout (in milliseconds) to both C++ and Python clients. - Add `ClientTest.testConnectTimeout` (C++) and `test_connect_timeout` (Python) and to verify the connection timeout works. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change added tests and can be verified as follows: - PeriodicTaskTest - ClientTest.testConnectTimeout - test_connect_timeout (cherry picked from commit 6062d2f)

…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)

Fixes apache#10747 ### Motivation This PR is a catchup of apache#2852 and adds connection timeout configuration to C++ and Python client. ### Modifications - Add a `PeriodicTask` class to execute tasks periodically and the relate unit tests: `PeriodicTastTest`. - Use `PeriodicTask` to register a timer before connecting to broker asynchronously, if the connection was not established when the timer is triggered, close the socket so that `handleTcpConnected` can be triggered immediately with a failure. - Add connection timeout (in milliseconds) to both C++ and Python clients. - Add `ClientTest.testConnectTimeout` (C++) and `test_connect_timeout` (Python) and to verify the connection timeout works. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change added tests and can be verified as follows: - PeriodicTaskTest - ClientTest.testConnectTimeout - test_connect_timeout

…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.

BewareMyPower added 7 commits June 22, 2021 22:51

Add PeriodicTask and its tests

a40a1a9

Add tests for negative timeout

3905b9c

Add connection timeout to ClientConfiguration

1e115ed

Use setCallback instead of virtual function

ef3ba31

Add connection timeout and tests

214ec97

Add connection timeout to Python client

384be1a

Register timer before another async_connect

fe7ef8b

BewareMyPower self-assigned this Jun 23, 2021

BewareMyPower added component/c++ labels Jun 23, 2021

BewareMyPower requested review from merlimat, massakam, aahmed-se, jiazhai, rdhabalia and sijie June 23, 2021 04:11

Avoid exception thrown when socket is closed

31eff3a

BewareMyPower changed the title ~~[C++][Python] Add connection timeout configuration~~ [WIP][C++][Python] Add connection timeout configuration Jun 23, 2021

freeznet reviewed Jun 24, 2021

View reviewed changes

pulsar-client-cpp/python/pulsar/__init__.py Outdated Show resolved Hide resolved

Fix Python Function that uses positional argument

96324ba

BewareMyPower changed the title ~~[WIP][C++][Python] Add connection timeout configuration~~ [C++][Python] Add connection timeout configuration Jun 24, 2021

freeznet approved these changes Jun 25, 2021

View reviewed changes

sijie added this to the 2.9.0 milestone Jun 25, 2021

sijie approved these changes Jun 25, 2021

View reviewed changes

sijie merged commit 6062d2f into apache:master Jun 25, 2021

BewareMyPower deleted the bewaremypower/add-cpp-connect-timeout branch June 27, 2021 10:21

BewareMyPower mentioned this pull request Jun 28, 2021

(2.7.2) Python client blocks forever in "create_producer" during a broker failure, even if "operation_timeout_seconds" is set #10721

Closed

BewareMyPower mentioned this pull request Aug 4, 2021

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

Merged

codelipenghui added release/2.8.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Aug 5, 2021

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 5, 2021

merlimat mentioned this pull request Oct 18, 2021

Python Pulsar client creates producer and then close producer, the memory will not be freed #12408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++][Python] Add connection timeout configuration #11029

[C++][Python] Add connection timeout configuration #11029

BewareMyPower commented Jun 23, 2021

BewareMyPower commented Jun 23, 2021

BewareMyPower commented Jun 23, 2021

freeznet left a comment

[C++][Python] Add connection timeout configuration #11029

[C++][Python] Add connection timeout configuration #11029

Conversation

BewareMyPower commented Jun 23, 2021

Motivation

Modifications

Verifying this change

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

Documentation

BewareMyPower commented Jun 23, 2021

BewareMyPower commented Jun 23, 2021

freeznet left a comment

Choose a reason for hiding this comment