Commons Dbcp
  1. Commons Dbcp
  2. DBCP-4

[dbcp] Use commons-logging for debugging instead of System.out.println

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: 1.3
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      At the source code of commons-dbcp, some System.out.println() and
      System.err.println() statements can be found, noticeably in the constructor of
      "java.org.apache.commons.dbcp.AbandonedObjectPool".

      These statements are annoying, because none of the developers want to see these
      messages but they occuplied the precious space in the log files.

      I think it is still appropriate to use this method to emit errors, but for
      normal behavior, we should have an option never seeing them.

        Issue Links

          Activity

          Hide
          Philip Jacob added a comment -

          Created an attachment (id=12689)
          patch to get commons-logging in dbcp

          Show
          Philip Jacob added a comment - Created an attachment (id=12689) patch to get commons-logging in dbcp
          Hide
          Alan Tam added a comment -
          • System.out.println("AbandonedObjectPool is used (" + this + ")");
          • System.out.println(" LogAbandoned: " + config.getLogAbandoned());
          • System.out.println(" RemoveAbandoned: " + config.getRemoveAbandoned());
          • System.out.println(" RemoveAbandonedTimeout: " +
            config.getRemoveAbandonedTimeout());
            + log.error("AbandonedObjectPool is used (" + this + ")");
            + log.error(" LogAbandoned: " + config.getLogAbandoned());
            + log.error(" RemoveAbandoned: " + config.getRemoveAbandoned());
            + log.error(" RemoveAbandonedTimeout: " +
            config.getRemoveAbandonedTimeout());

          I suppose they are info, or even debug.

          • System.err.println("Could not parse
            defaultTransactionIsolation: " + value);
          • System.err.println("WARNING: defaultTransactionIsolation
            not set");
          • System.err.println("using default value of database driver");
            + log.error("Could not parse defaultTransactionIsolation: " +
            value);
            + log.error("WARNING: defaultTransactionIsolation not set");
            + log.error("using default value of database driver");

          I think they are warning instead.

          Show
          Alan Tam added a comment - System.out.println("AbandonedObjectPool is used (" + this + ")"); System.out.println(" LogAbandoned: " + config.getLogAbandoned()); System.out.println(" RemoveAbandoned: " + config.getRemoveAbandoned()); System.out.println(" RemoveAbandonedTimeout: " + config.getRemoveAbandonedTimeout()); + log.error("AbandonedObjectPool is used (" + this + ")"); + log.error(" LogAbandoned: " + config.getLogAbandoned()); + log.error(" RemoveAbandoned: " + config.getRemoveAbandoned()); + log.error(" RemoveAbandonedTimeout: " + config.getRemoveAbandonedTimeout()); I suppose they are info, or even debug. System.err.println("Could not parse defaultTransactionIsolation: " + value); System.err.println("WARNING: defaultTransactionIsolation not set"); System.err.println("using default value of database driver"); + log.error("Could not parse defaultTransactionIsolation: " + value); + log.error("WARNING: defaultTransactionIsolation not set"); + log.error("using default value of database driver"); I think they are warning instead.
          Hide
          Philip Jacob added a comment -

          Created an attachment (id=12690)
          patch taking alan's comments into consideration, plus more use of commons logging

          Show
          Philip Jacob added a comment - Created an attachment (id=12690) patch taking alan's comments into consideration, plus more use of commons logging
          Hide
          Alan Tam added a comment -

          Any idea when it can be checked in?

          Show
          Alan Tam added a comment - Any idea when it can be checked in?
          Hide
          Dirk Verbeeck added a comment -

          This patch introduces a new dependency on commons-logging.
          I'm not planning to commit this patch because of this new dependency.
          If the dependency is made optional (runtime detection unsing refection) then
          I'll maybe consider it.

          Another (and maybe better) option is to use DriverManager.getLogWriter() and log
          using this writer.

          Show
          Dirk Verbeeck added a comment - This patch introduces a new dependency on commons-logging. I'm not planning to commit this patch because of this new dependency. If the dependency is made optional (runtime detection unsing refection) then I'll maybe consider it. Another (and maybe better) option is to use DriverManager.getLogWriter() and log using this writer.
          Hide
          Bruce Atherton added a comment -

          I'd like to disagree with the logic in the rejection of this patch, and I hope I
          can pursuade you to change your mind.

          Either DBCP should log information or it shouldn't. If it should, then it should
          be using a logging package to provide that behaviour rather than trying to do
          the job poorly itself. If DBCP should not log information, then the System.out
          and System.err lines should be removed altogether.

          The commons-logging package is extremely lightweight, and is a good choice for a
          dependency in a library like DBCP that is itself fairly low level. Think of it
          as the set of classes to do reflection that are suggested in the rejection of
          the patch.

          Why this matters is that libraries putting anything on stdout or stderr is an
          Antipattern. It is a side effect that the user of the library can't control. It
          is especially incomprehensible in text-mode programs. Consider this dialog:

          bash$ animal
          GUESS WHAT KIND OF ANIMAL I AM
          Pick an animal for you to be. I'll ask you a series of questions.
          Answer YES or NO to each one, and I'll guess what kind you are.
          AbandonedObjectPool is used (org.apache.commons.dbcp.AbandonedObjectPool@113f501)
          LogAbandoned: false
          RemoveAbandoned: true
          RemoveAbandonedTimeout: 300
          Are you a mammal?

          If you absolutely must keep the output, and you absolutely refuse to send it to
          a logging package where it belongs, at least give a configuration option to
          suppress the output to stdout and stderr, please.

          Show
          Bruce Atherton added a comment - I'd like to disagree with the logic in the rejection of this patch, and I hope I can pursuade you to change your mind. Either DBCP should log information or it shouldn't. If it should, then it should be using a logging package to provide that behaviour rather than trying to do the job poorly itself. If DBCP should not log information, then the System.out and System.err lines should be removed altogether. The commons-logging package is extremely lightweight, and is a good choice for a dependency in a library like DBCP that is itself fairly low level. Think of it as the set of classes to do reflection that are suggested in the rejection of the patch. Why this matters is that libraries putting anything on stdout or stderr is an Antipattern. It is a side effect that the user of the library can't control. It is especially incomprehensible in text-mode programs. Consider this dialog: bash$ animal GUESS WHAT KIND OF ANIMAL I AM Pick an animal for you to be. I'll ask you a series of questions. Answer YES or NO to each one, and I'll guess what kind you are. AbandonedObjectPool is used (org.apache.commons.dbcp.AbandonedObjectPool@113f501) LogAbandoned: false RemoveAbandoned: true RemoveAbandonedTimeout: 300 Are you a mammal? If you absolutely must keep the output, and you absolutely refuse to send it to a logging package where it belongs, at least give a configuration option to suppress the output to stdout and stderr, please.
          Hide
          Henri Yandell added a comment -

          Or move to JDK 1.4 as a minimum supported version of Java and use standard logging.

          Show
          Henri Yandell added a comment - Or move to JDK 1.4 as a minimum supported version of Java and use standard logging.
          Hide
          Madhusudan Pagadala added a comment -

          This is very very important to change to commons-logging, also I prefer to add some more debug or info statements like, creating new connections and closing old connections, and properties used by the pooling, this type of logging should be there to optimize DB connections in pools, and debug the production problems, this package is causing vey annoying because of there is no logging, EVEN I AM THINKING OF OWNING COPY OF DBCP SOURCE and ADD LOGGING, finally Decided to raise this as an issue, with fouram, I also created a JIRA, but this seems already sheduled to 1.3, It will be very nice, THIS MESSAGE NOT ONLY MINE, IN MY OFFICE NEARLY 100 DEVELOPERS VOTED FOR THIS CHANGE, I AM SENDING YOU THIS MESSAGE BEHALF ALL OF THEM.

          I AM STRONGLY SUPPORT THIS CHANGE WITH 100/100 PREIORITY.

          Thanks,
          Madhu.

          Show
          Madhusudan Pagadala added a comment - This is very very important to change to commons-logging, also I prefer to add some more debug or info statements like, creating new connections and closing old connections, and properties used by the pooling, this type of logging should be there to optimize DB connections in pools, and debug the production problems, this package is causing vey annoying because of there is no logging, EVEN I AM THINKING OF OWNING COPY OF DBCP SOURCE and ADD LOGGING, finally Decided to raise this as an issue, with fouram, I also created a JIRA, but this seems already sheduled to 1.3, It will be very nice, THIS MESSAGE NOT ONLY MINE, IN MY OFFICE NEARLY 100 DEVELOPERS VOTED FOR THIS CHANGE, I AM SENDING YOU THIS MESSAGE BEHALF ALL OF THEM. I AM STRONGLY SUPPORT THIS CHANGE WITH 100/100 PREIORITY. Thanks, Madhu.
          Hide
          Madhusudan Pagadala added a comment -

          Anyone working on this issue??

          Show
          Madhusudan Pagadala added a comment - Anyone working on this issue??
          Hide
          Jonathan Amir added a comment -

          yeah, I vote for fixing this as well. It doesn't make sense that a commons library won't use commons-logging and instead throw its output to stdout or stderr. Very annoying.

          Show
          Jonathan Amir added a comment - yeah, I vote for fixing this as well. It doesn't make sense that a commons library won't use commons-logging and instead throw its output to stdout or stderr. Very annoying.
          Hide
          Mark Thomas added a comment -

          Of the places were System.out is currently used:

          • AbandonedObjectpool and AbandonedTrace have been deprecated
          • The remaining uses are for the default logWriter which may be replaced if required by the developer

          Therefore, I see no need for any changes to DBCP.

          Show
          Mark Thomas added a comment - Of the places were System.out is currently used: AbandonedObjectpool and AbandonedTrace have been deprecated The remaining uses are for the default logWriter which may be replaced if required by the developer Therefore, I see no need for any changes to DBCP.
          Hide
          Joshua Spiewak added a comment -

          AbandonedObjectpool and AbandonedTrace have been deprecated for at least the same 4.5 years that this issue has been opened. A quick look at the 1.3 JIRA issues did not turn up something replacing them. You are trading off your desire to not introduce a dependency with the consumers of the library being forced to use System.out as part of their logging system that they must monitor.

          Show
          Joshua Spiewak added a comment - AbandonedObjectpool and AbandonedTrace have been deprecated for at least the same 4.5 years that this issue has been opened. A quick look at the 1.3 JIRA issues did not turn up something replacing them. You are trading off your desire to not introduce a dependency with the consumers of the library being forced to use System.out as part of their logging system that they must monitor.
          Hide
          Mark Thomas added a comment -

          I have been reading through the archives. There may be a case for removing the deprecation in which case the System.out usage will need to be fixed. I want to look at this some more so am re-opening so I don't forget to come back to it.

          Whatever the end result, I am pretty certain commons-logging won't be the solution. Using the logWriters looks like a better bet.

          Show
          Mark Thomas added a comment - I have been reading through the archives. There may be a case for removing the deprecation in which case the System.out usage will need to be fixed. I want to look at this some more so am re-opening so I don't forget to come back to it. Whatever the end result, I am pretty certain commons-logging won't be the solution. Using the logWriters looks like a better bet.
          Hide
          Mark Thomas added a comment -

          Compromise patch applied.

          • Logging to stdout on pool creation was removed as a generally bad idea and inconsistent with the other pool impls
          • Logging location of traces was made configurable. Developers using DBCP directly can change the PrintWriter. App servers embedding DBCP (such as Tomcat) will probably need to retain their mechanisms for handling libraries that use stdout.
          Show
          Mark Thomas added a comment - Compromise patch applied. Logging to stdout on pool creation was removed as a generally bad idea and inconsistent with the other pool impls Logging location of traces was made configurable. Developers using DBCP directly can change the PrintWriter. App servers embedding DBCP (such as Tomcat) will probably need to retain their mechanisms for handling libraries that use stdout.

            People

            • Assignee:
              Unassigned
              Reporter:
              Alan Tam
            • Votes:
              5 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development