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/12/23 09:54:12 UTC
[GitHub] [pulsar-client-cpp] RobertIndie opened a new pull request, #156: [fix] Fix log for connection disconnected expectedly
RobertIndie opened a new pull request, #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
Fixes #140
### Motivation
When running produce and consume, and the client calls close, it
shows `ConnectError` in the close method
```
[[127.0.0.1:43644](http://127.0.0.1:43644/) -> [127.0.0.1:6650](http://127.0.0.1:6650/)] Connection closed with
ConnectError
```
### Modifications
* Show `Connection disconnected` just like the java client did instead of `ConnectError` when the client disconnect successfully.
* Add error log for unexpected closed connection.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `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-client-cpp] shibd commented on a diff in pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156#discussion_r1058106366
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
As you provided the example, The call `close` may have been an unexpected behavior, so we printed the error log before the call. But inside the `close` method, the execution close method behaves as expected and is closed correctly.
--
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-client-cpp] shibd commented on a diff in pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156#discussion_r1057226702
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
When the result is `ResultRetryable`, is there no need to print the error log here?
BTW: I think just print the info level log here because calling close is the expected behavior of the code.
--
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-client-cpp] shibd commented on a diff in pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156#discussion_r1058138233
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
Okay, I agree.
--
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-client-cpp] RobertIndie commented on a diff in pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156#discussion_r1058133945
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
It will pass the default result value `ResultConnectError` to the `close()` to indicate that the connection is closed unexpectedly.
In addition, the info log with the `ConnectError` message seems confusing.
--
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-client-cpp] RobertIndie commented on a diff in pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156#discussion_r1058094316
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
Thanks. I changed it to print info log when the result is ResultRetryable.
> calling close is the expected behavior of the code.
Many calling of the close is unexpected behavior. such as：
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L485-L491
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L213-L219
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L223-L229
etc...
##########
lib/ClientConnection.cc:
##########
@@ -1596,7 +1596,11 @@ void ClientConnection::close(Result result) {
}
lock.unlock();
- LOG_INFO(cnxString_ << "Connection closed with " << result);
+ if (result != ResultDisconnected) {
+ LOG_ERROR(cnxString_ << "Connection closed with " << result);
Review Comment:
Thanks. I changed it to print info log when the result is ResultRetryable.
> calling close is the expected behavior of the code.
Many calling of the close is unexpected behavior. such as：
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L485-L491
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L213-L219
https://github.com/apache/pulsar-client-cpp/blob/5d5ed2acc2e89aed90b593343369f360644440e7/lib/ClientConnection.cc#L223-L229
etc...
--
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-client-cpp] BewareMyPower merged pull request #156: [fix] Fix log for connection disconnected expectedly
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #156:
URL: https://github.com/apache/pulsar-client-cpp/pull/156
--
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