You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/08/08 09:10:52 UTC

[GitHub] [hbase] mokai87 opened a new pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

mokai87 opened a new pull request #2219:
URL: https://github.com/apache/hbase/pull/2219


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] busbey commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
busbey commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r471028927



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       For background, I left a longer comment awhile ago on HBASE-24686 where I asked for some indication of the close at info and the stack trace at debug.
   
   we already have other things that get logged at INFO when a connection is closing, but it's incidental and could happen in other ways. so an INFO message that clearly demarcates connection close is a thing I *really* want from this change. Enough so that I am sure I'll be back requesting it again the very next time I run into a production problem where I don't see the message, because almost no one I support runs DEBUG in production as a matter of course. Most of the times where this log message would have helped me in the last ~6 months the thread name and an unambiguous "we closed the connection" would suffice.
   
   the stack trace would help with a specific problem we hit in a dev set up with an apparent bug in the hmaster. It did not happen often but it did happen regularly, so changing the log level to DEBUG to see a stack trace would have worked fine for us.
   
   If you feel strongly about not having the messages at different log levels, @virajjasani, I'd rather see both at INFO then both at DEBUG, even though I generally am against non-error related stack traces at log levels above DEBUG.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] Apache-HBase commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-670852720


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 25s |  Docker mode activated.  |
   ||| _ Prechecks _ |
   | +1 :green_heart: |  dupname  |   0m  0s |  No case conflicting files found.  |
   | +1 :green_heart: |  hbaseanti  |   0m  0s |  Patch does not have any anti-patterns.  |
   | +1 :green_heart: |  @author  |   0m  0s |  The patch does not contain any @author tags.  |
   ||| _ master Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   3m 38s |  master passed  |
   | +1 :green_heart: |  checkstyle  |   0m 28s |  master passed  |
   | +1 :green_heart: |  spotbugs  |   1m  1s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   3m 20s |  the patch passed  |
   | +1 :green_heart: |  checkstyle  |   0m 26s |  the patch passed  |
   | +1 :green_heart: |  whitespace  |   0m  0s |  The patch has no whitespace issues.  |
   | +1 :green_heart: |  hadoopcheck  |  10m 53s |  Patch does not cause any errors with Hadoop 3.1.2 3.2.1.  |
   | +1 :green_heart: |  spotbugs  |   1m  7s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  asflicense  |   0m 13s |  The patch does not generate ASF License warnings.  |
   |  |   |  28m 26s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/artifact/yetus-general-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2219 |
   | Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle |
   | uname | Linux f066bc09a580 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 3edfa1025c |
   | Max. process+thread count | 94 (vs. ulimit of 12500) |
   | modules | C: hbase-client U: hbase-client |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/console |
   | versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
   | Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470969961



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       Sorry for lack of clarification in previous comment. Also, changes look good, just that once we have both at `info` level, we should be good to go ahead.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r471075141



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       > Most of the times where this log message would have helped me in the last ~6 months the thread name and an unambiguous "we closed the connection" would suffice.
   
   That's perfectly fine, if thread name is enough for prod env and if we have a way to repro in dev env, we can let stacktrace be at `debug`. The only thing I was concerned about was whether not finding stacktrace in prod env would still keep things unclear for user. However, here I believe once we get the thread name, we can find recent logs of that thread to find more.
   
   > I generally am against non-error related stack traces at log levels above DEBUG.
   
   +1 for this unless the non-error trace is useful for detailed debugging, in which case maybe we can write something specifically to indicate to not treat this as erroneous condition.
   However, for our current PR, it seems thread-name is most likely going to provide sufficient info.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani edited a comment on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani edited a comment on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-674889021


   @busbey just to confirm, you have +1 for this PR right? (I mean no other concerns than DEBUG and INFO level discussions)


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470970085



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       Oh wait, how about first we gather stack trace and then log both `threadname` and `stacktrace` together in same log?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] Apache-HBase commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-670852390


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   0m 27s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   4m 20s |  master passed  |
   | +1 :green_heart: |  compile  |   0m 29s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   5m 47s |  branch has no errors when building our shaded downstream artifacts.  |
   | -0 :warning: |  javadoc  |   0m 28s |  hbase-client in master failed.  |
   ||| _ Patch Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   4m  0s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 28s |  the patch passed  |
   | +1 :green_heart: |  javac  |   0m 28s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   5m 42s |  patch has no errors when building our shaded downstream artifacts.  |
   | -0 :warning: |  javadoc  |   0m 26s |  hbase-client in the patch failed.  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  9s |  hbase-client in the patch passed.  |
   |  |   |  24m 29s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2219 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux 0b5a016db29e 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 3edfa1025c |
   | Default Java | 2020-01-14 |
   | javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-client.txt |
   | javadoc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-client.txt |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/testReport/ |
   | Max. process+thread count | 298 (vs. ulimit of 12500) |
   | modules | C: hbase-client U: hbase-client |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/console |
   | versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
   | Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] mokai87 commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
mokai87 commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-674823709


   @busbey @pankaj72981 @virajjasani Thanks for the reviews. Who can help to merge this PR if there is no more comment?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] Apache-HBase commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-670852412


   :confetti_ball: **+1 overall**
   
   
   
   
   
   
   | Vote | Subsystem | Runtime | Comment |
   |:----:|----------:|--------:|:--------|
   | +0 :ok: |  reexec  |   1m  3s |  Docker mode activated.  |
   | -0 :warning: |  yetus  |   0m  3s |  Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck  |
   ||| _ Prechecks _ |
   ||| _ master Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   4m  5s |  master passed  |
   | +1 :green_heart: |  compile  |   0m 24s |  master passed  |
   | +1 :green_heart: |  shadedjars  |   6m  7s |  branch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 23s |  master passed  |
   ||| _ Patch Compile Tests _ |
   | +1 :green_heart: |  mvninstall  |   3m 46s |  the patch passed  |
   | +1 :green_heart: |  compile  |   0m 23s |  the patch passed  |
   | +1 :green_heart: |  javac  |   0m 23s |  the patch passed  |
   | +1 :green_heart: |  shadedjars  |   6m  2s |  patch has no errors when building our shaded downstream artifacts.  |
   | +1 :green_heart: |  javadoc  |   0m 21s |  the patch passed  |
   ||| _ Other Tests _ |
   | +1 :green_heart: |  unit  |   1m  8s |  hbase-client in the patch passed.  |
   |  |   |  24m 46s |   |
   
   
   | Subsystem | Report/Notes |
   |----------:|:-------------|
   | Docker | Client=19.03.12 Server=19.03.12 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
   | GITHUB PR | https://github.com/apache/hbase/pull/2219 |
   | Optional Tests | javac javadoc unit shadedjars compile |
   | uname | Linux ae29f06a63ea 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
   | Build tool | maven |
   | Personality | dev-support/hbase-personality.sh |
   | git revision | master / 3edfa1025c |
   | Default Java | 1.8.0_232 |
   |  Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/testReport/ |
   | Max. process+thread count | 221 (vs. ulimit of 12500) |
   | modules | C: hbase-client U: hbase-client |
   | Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2219/1/console |
   | versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
   | Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
   
   
   This message was automatically generated.
   
   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani merged pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani merged pull request #2219:
URL: https://github.com/apache/hbase/pull/2219


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-674889021


   @busbey just to confirm, you have +1 for this PR right?


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] pankaj72981 commented on pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
pankaj72981 commented on pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#issuecomment-671412303


   @busbey  please have a look.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] busbey commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
busbey commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470927873



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       I'd also like to keep this at INFO. Connection objects are supposed to be long lived so we should see this message rarely. Ideally once per cluster per JVM.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] mokai87 commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
mokai87 commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470926641



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       It is helpful to identify who closed the connection improperly in  production env. 
   Level debug is inconvenient since we have to re-run the user application to reproduce it. Meanwhile, it should not be called frequently.
   Maybe I did not get your concern.
   




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470969543



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       I didn't mean to correlate with no of occurrences of this log, I meant to say user who is interested to know which thread closed the connection would also likely want to check out `call stacktrace`. So if call stack is at `debug` level, let's also keep thread name at `debug` level.
   
   But yeah, not everyone enables `debug` log for production use-case and hence, maybe we should keep both at `info` level? Stacktrace is also equally interesting right?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r471012031



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       > I'm also worried log stack at info level is confusing since the format is like an exception.
   
   Even though it is Exception, but we do want to log which Exception caused Connection closure right? If so, then keeping threadname at info level and stacktrace at debug somehow doesn't fit well IMHO.
   If we know for certain that threadname is useful to get sufficient info, then we should not log stacktrace at all, and if stacktrace is required for debugging, then it should be at the same level as threadname.
   
   > Level debug is inconvenient since we have to re-run the user application to reproduce it.
   
   This is exactly applicable to stacktrace also. If application is running with `info` level and if threadname is not sufficient to know the cause, then we can't get stacktrace either because our application was running with `info` level at that time and stacktrace is present at `debug` level in 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r471012031



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       > I'm also worried log stack at info level is confusing since the format is like an exception.
   
   Even though it is Exception, but we do want to log which Exception caused Connection closure (once in a while) right? If so, then keeping threadname at info level and stacktrace at debug somehow doesn't fit well IMHO.
   If we know for certain that threadname is useful to get sufficient info, then we should not log stacktrace at all, and if stacktrace is required for debugging, then it should be at the same level as threadname.
   
   > Level debug is inconvenient since we have to re-run the user application to reproduce it.
   
   This is exactly applicable to stacktrace also. If application is running with `info` level and if threadname is not sufficient to know the cause, then we can't get stacktrace either because our application was running with `info` level at that time and stacktrace is present at `debug` level in 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] virajjasani commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470737747



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       I think this should also be at `debug` level.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [hbase] mokai87 commented on a change in pull request #2219: HBASE-24686 [LOG] Log improvement in Connection#close

Posted by GitBox <gi...@apache.org>.
mokai87 commented on a change in pull request #2219:
URL: https://github.com/apache/hbase/pull/2219#discussion_r470995249



##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -199,6 +199,10 @@ public void close() {
     if (!closed.compareAndSet(false, true)) {
       return;
     }
+    LOG.info("Connection has been closed by {}.", Thread.currentThread().getName());

Review comment:
       I think the stacktrace is an option if the thread name is helpless. It's almost enough with meaningful name. I'm also worried log stack at info level is confusing since the format is like an exception.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org