You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/01 17:04:04 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request, #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
BewareMyPower opened a new pull request, #17410:
URL: https://github.com/apache/pulsar/pull/17410
### Motivation
Currently the operation timeout only works for requests other than
lookup, like SEND and FLOW. However, the lookup requests, which are sent
by `LookupService`, should also apply the operation timeout, see
https://github.com/apache/pulsar/blob/7075a5ce0d4a70f52625ac8c3d0c48894442b72a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L1019-L1025
In addition, no attempts would be retried if lookup failed even due to a
retryable reason. For example, if some of all configured brokers were
down, the C++ client would fail immediately.
Therefore, this PR intends to retry lookup for some certain cases:
- The connection cannot be established, except connection timeout, which
is controlled by the connection timeout.
- A `ServiceNotReady` error is received, except it's caused by
`PulsarServerException`, e.g. the listener name is wrong.
Then, apply the operation timeout to avoid infinite retries.
### Modifications
- Add a `ResultRetryable` error code, which should only be used
internally. Complete the futures with this error in the cases said
previously.
- Add a `RetryableLookupService` implementation to support retries based
on the backoff policy. Replace the directly usages of `LookupService`
implementations with this class in `ClientImpl`.
The following tests are added:
- `ClientTest.testMultiBrokerUrl`: verify when multiple brokers are
configured, even if one of them is not available, the creation of
producer or consumer could still succeed.
- `LookupService.testRetry`: verify all lookup methods could be retried.
- `LookupService.testTimeout`: verify all lookup methods could be
completed with `ResultTimeout` if no brokers are available.
### TODO
In future, we should add lookup timeout instead of operation timeout for
lookup requests and separate lookup connection pool, see PIP-91.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970260032
##########
pulsar-client-cpp/lib/ClientConnection.cc:
##########
@@ -1557,34 +1567,34 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed");
+ LOG_INFO(cnxString_ << "Connection closed with " << result);
for (ProducersMap::iterator it = producers.begin(); it != producers.end(); ++it) {
- HandlerBase::handleDisconnection(ResultConnectError, shared_from_this(), it->second);
+ HandlerBase::handleDisconnection(result, shared_from_this(), it->second);
Review Comment:
When this call `handleDisconnection` and param `result == ResultRetryable`, `handleDisconnection` method should handle `ResultRetryable`?
https://github.com/apache/pulsar/blob/fb0f653eadcf6bf72eb8c8efcc29975da6e21267/pulsar-client-cpp/lib/HandlerBase.cc#L108-L121
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r971023591
##########
pulsar-client-cpp/lib/ClientConnection.cc:
##########
@@ -1557,34 +1567,34 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed");
+ LOG_INFO(cnxString_ << "Connection closed with " << result);
for (ProducersMap::iterator it = producers.begin(); it != producers.end(); ++it) {
- HandlerBase::handleDisconnection(ResultConnectError, shared_from_this(), it->second);
+ HandlerBase::handleDisconnection(result, shared_from_this(), it->second);
Review Comment:
Addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970902167
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
+ public std::enable_shared_from_this<RetryableLookupService> {
+ private:
+ friend class PulsarFriend;
+ struct PassKey {
+ explicit PassKey() {}
+ };
+
+ public:
+ template <typename... Args>
+ explicit RetryableLookupService(PassKey, Args&&... args)
+ : RetryableLookupService(std::forward<Args>(args)...) {}
+
+ template <typename... Args>
+ static std::shared_ptr<RetryableLookupService> create(Args&&... args) {
Review Comment:
Why use `...args`? Maybe it's clearer to use the exact param?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#issuecomment-1240275564
@merlimat @Demogorgon314 @RobertIndie PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970906640
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
Review Comment:
We usually create a source file (*.cc) to reduce compilation time. For example, if
- a.cc includes common.h and a.h
- b.cc includes common.h and b.h
If common.h changed, a.cc and b.cc would be compiled again. If common.h changed only because some method implementations changed, we can use another common.cc for them. Then only common.cc needs to be compiled because common.h, which only includes the API signatures that don't change, doesn't change.
This `RetryableLookupService.h` is only used in `ClientImpl.cc` so separating the declarations and definitions is not necessary. And methods defined in the header are inline by default so they are faster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r971418996
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
Review Comment:
Get it, thanks for your explain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 merged pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
Demogorgon314 merged PR #17410:
URL: https://github.com/apache/pulsar/pull/17410
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970907655
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
Review Comment:
BTW, the most complicated method is a template method, which cannot be separated into another source file. Other methods are very simple so it's better to make them inline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970898507
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
Review Comment:
Does the implementation of this class need to create a separate cc file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#issuecomment-1247549794
ping @merlimat @Demogorgon314 @RobertIndie
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#issuecomment-1234581163
@BewareMyPower Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970916981
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
+ public std::enable_shared_from_this<RetryableLookupService> {
+ private:
+ friend class PulsarFriend;
+ struct PassKey {
+ explicit PassKey() {}
+ };
+
+ public:
+ template <typename... Args>
+ explicit RetryableLookupService(PassKey, Args&&... args)
+ : RetryableLookupService(std::forward<Args>(args)...) {}
+
+ template <typename... Args>
+ static std::shared_ptr<RetryableLookupService> create(Args&&... args) {
Review Comment:
It's a template code that leverages perfect forwarding to implement Passkey idiom. This wrapper will still work If the constructor's signature changed in future.
BTW, you can see the following blogs to learn more about the Passkey idiom:
- https://www.spiria.com/en/blog/desktop-software/passkey-idiom-and-better-friendship-c/
- https://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-class-with-only-protected-or-private-const
- (Chinese) https://bewaremypower.github.io/2019/04/14/make-shared%E8%B0%83%E7%94%A8%E7%A7%81%E6%9C%89%E6%9E%84%E9%80%A0%E5%87%BD%E6%95%B0%E7%9A%84%E8%A7%A3%E5%86%B3%E6%96%B9%E6%B3%95/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17410: [fix][cpp] Support retry and apply operation timeout for lookup requests
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17410:
URL: https://github.com/apache/pulsar/pull/17410#discussion_r970916981
##########
pulsar-client-cpp/lib/RetryableLookupService.h:
##########
@@ -0,0 +1,151 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+#pragma once
+
+#include <algorithm>
+#include <memory>
+#include "lib/Backoff.h"
+#include "lib/ExecutorService.h"
+#include "lib/LookupService.h"
+#include "lib/SynchronizedHashMap.h"
+#include "lib/LogUtils.h"
+
+namespace pulsar {
+
+class RetryableLookupService : public LookupService,
+ public std::enable_shared_from_this<RetryableLookupService> {
+ private:
+ friend class PulsarFriend;
+ struct PassKey {
+ explicit PassKey() {}
+ };
+
+ public:
+ template <typename... Args>
+ explicit RetryableLookupService(PassKey, Args&&... args)
+ : RetryableLookupService(std::forward<Args>(args)...) {}
+
+ template <typename... Args>
+ static std::shared_ptr<RetryableLookupService> create(Args&&... args) {
Review Comment:
It's a template code that leverages perfect forwarding to implement Passkey idiom. This wrapper will still work even if the constructor's signature changed in future.
BTW, you can see the following blogs to learn more about the Passkey idiom:
- https://www.spiria.com/en/blog/desktop-software/passkey-idiom-and-better-friendship-c/
- https://stackoverflow.com/questions/8147027/how-do-i-call-stdmake-shared-on-a-class-with-only-protected-or-private-const
- (Chinese) https://bewaremypower.github.io/2019/04/14/make-shared%E8%B0%83%E7%94%A8%E7%A7%81%E6%9C%89%E6%9E%84%E9%80%A0%E5%87%BD%E6%95%B0%E7%9A%84%E8%A7%A3%E5%86%B3%E6%96%B9%E6%B3%95/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org