Uploaded image for project: 'Qpid'
  1. Qpid
  2. QPID-8291

Inconsistent log level practices in source code

    Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: qpid-java-6.1.7
    • Fix Version/s: None
    • Component/s: Broker-J
    • Labels:

      Description

      Hi,
      I found there are inconsistent log level practices in the Qpid project, and we suspect some of them should be fixed.
      We select 4 problematic practices to report.
      The detail code as well as the modification suggestions are shown below.

      ***************** Report1 *********************************
      the problematic snippet:
      ============ ReplicatedEnvironmentFacade.java ===================
      file path: qpid-java-6.1.7\bdbstore\src\main\java\org\apache\qpid\server\store\berkeleydb\replication\ReplicatedEnvironmentFacade.java
      logging statement line: 916
      modification suggestion: change log level to WARN
       890         try
       891         {
       892             return future.get(timeout, TimeUnit.SECONDS);
       893         }
       894         catch (InterruptedException e)
       895         {
       896             Thread.currentThread().interrupt();
       897         }
       898         catch (ExecutionException e)
       899         {
       900             Throwable cause = e.getCause();
       901             if (cause instanceof Error)
       902             {
       903                 throw (Error) cause;
       904             }
       905             else if (cause instanceof RuntimeException)
       906             {
       907                 throw (RuntimeException) cause;
       908             }
       909             else
       910             {
       911                 throw new ConnectionScopedRuntimeException("Unexpected exception while " + action, e);
       912             }
       913         }
       914         catch (TimeoutException e)
       915         {
       916             LOGGER.info("{}  on {} timed out after {} seconds", action, _prettyGroupNodeName, timeout);
       917         }
      
      
      the similar snippet:
      ============ SelectorThread.java ===============================
      file path: qpid-java-6.1.7\broker-core\src\main\java\org\apache\qpid\server\transport\SelectorThread.java
      logging statement line: 464
      449         try
      450         {
      451             result.get(ACCEPT_CANCELATION_TIMEOUT, TimeUnit.MILLISECONDS);
      452         }
      453         catch (InterruptedException e)
      454         {
      455             LOGGER.warn("Cancellation of accepting socket was interrupted");
      456             Thread.currentThread().interrupt();
      457         }
      458         catch (ExecutionException e)
      459         {
      460             LOGGER.warn("Cancellation of accepting socket failed", e.getCause());
      461         }
      462         catch (TimeoutException e)
      463         {
      464             LOGGER.warn("Cancellation of accepting socket timed out");
      465         }
      
      
      ============ BDBHAVirtualHostNodeImpl.java ====================
      file path: qpid-java-6.1.7\bdbstore\src\main\java\org\apache\qpid\server\virtualhostnode\berkeleydb\BDBHAVirtualHostNodeImpl.java
      logging statement line: 912
       906         try
       907         {
       908             future.get(MUTATE_JE_TIMEOUT_MS, TimeUnit.MILLISECONDS);
       909         }
       910         catch (TimeoutException e)
       911         {
       912             LOGGER.warn(timeoutLogMessage);
       913         }
      
      
      
      
      ***************** Report2 *********************************
      the problematic snippet:
      ============ AbstractJDBCPreferenceStore.java ===================
      file path: qpid-java-6.1.7\broker-core\src\main\java\org\apache\qpid\server\store\preferences\AbstractJDBCPreferenceStore.java 
      logging statement line: 334
      modification suggestion: change log level to ERROR 
      325             try
      326             {
      327                 if (connection != null)
      328                 {
      329                     connection.close();
      330                 }
      331             }
      332             catch (SQLException e)
      333             {
      334                 getLogger().warn("Failed to close JDBC connection", e);
      335             }
      
      
      the similar snippets:
      ============ JdbcUtils.java ==================================
      file path: qpid-java-6.1.7\broker-core\src\main\java\org\apache\qpid\server\store\JdbcUtils.java
      logging statement line: 42
       36             try
       37             {
       38                 conn.close();
       39             }
       40             catch (SQLException e)
       41             {
       42                 logger.error("Problem closing connection", e);
       43             }
      
      
      ***************** Report3 *********************************
      the problematic snippet:
      ============ DojoHelper.java =================================
      file path: qpid-java-6.1.7\broker-plugins\management-http\src\main\java\org\apache\qpid\server\management\plugin\DojoHelper.java
      logging statement line: 75
      modification suggestion: change log level to ERROR
       69                     try
       70                     {
       71                         propertyStream.close();
       72                     }
       73                     catch (IOException e)
       74                     {
       75                         _logger.warn("Exception closing InputStream for " + VERSION_FILE + " resource:", e);
       76                     }
      
      
      the similar snippet:
      ============ CallbackHandlerRegistry.java =======================
      file path: qpid-java-6.1.7\client\src\main\java\org\apache\qpid\client\security\CallbackHandlerRegistry.java
      logging statement line: 112
      105                 try
      106                 {
      107                     is.close();
      108
      109                 }
      110                 catch (IOException e)
      111                 {
      112                     _logger.error("Unable to close properties stream: " + e, e);
      113                 }
      
      
      ============ DynamicSaslRegistrar.java =========================
      file path: qpid-java-6.1.7\client\src\main\java\org\apache\qpid\client\security\DynamicSaslRegistrar.java
      logging statement line: 143
      136                 try
      137                 {
      138                     is.close();
      139
      140                 }
      141                 catch (IOException e)
      142                 {
      143                     _logger.error("Unable to close properties stream: " + e, e);
      144                 }
      
      
      ***************** Report4 *********************************
      the problematic snippet:
      ============ SSLUtil.java ====================================
      file path: qpid-java-6.1.7\common\src\main\java\org\apache\qpid\transport\network\security\ssl\SSLUtil.java
      logging statement line: 231
      modification suggestion: change log level to ERROR
      206             try
      207             {
      208                 LdapName ln = new LdapName(dn);
      209                 for(Rdn rdn : ln.getRdns())
      210                 {
      211                     if("CN".equalsIgnoreCase(rdn.getType()))
      212                     {
      213                         cnStr = rdn.getValue().toString();
      214                     }
      215                     else if("DC".equalsIgnoreCase(rdn.getType()))
      216                     {
      217                         if(dcStr == null)
      218                         {
      219                             dcStr = rdn.getValue().toString();
      220                         }
      221                         else
      222                         {
      223                             dcStr = rdn.getValue().toString() + '.' + dcStr;
      224                         }
      225                     }
      226                 }
      227                 return cnStr == null || cnStr.length()==0 ? "" : dcStr == null ? cnStr : cnStr + '@' + dcStr;
      228             }
      229             catch (InvalidNameException e)
      230             {
      231                 LOGGER.warn("Invalid name: '{}'", dn);
      232                 return "";
      233             }
      
      
      the similar snippet:
      ============ NonJavaTrustStoreImpl.java ==========================
      file path: qpid-java-6.1.7\broker-core\src\main\java\org\apache\qpid\server\security\NonJavaTrustStoreImpl.java
      logging statement line: 163
      147         try
      148         {
      149             LdapName ldapDN = new LdapName(dn);
      150             name = dn;
      151             for (Rdn rdn : ldapDN.getRdns())
      152             {
      153                 if (rdn.getType().equalsIgnoreCase("CN"))
      154                 {
      155                     name = String.valueOf(rdn.getValue());
      156                     break;
      157                 }
      158             }
      159
      160         }
      161         catch (InvalidNameException e)
      162         {
      163             LOGGER.error("Error getting subject name from certificate");
      164             name =  null;
      165         }
      
      
      ============ NonJavaKeyStoreImpl.java =========================
      file path: qpid-java-6.1.7\broker-core\src\main\java\org\apache\qpid\server\security\NonJavaKeyStoreImpl.java
      logging statement line: 132
      115             try
      116             {
      117                 String dn = _certificate.getSubjectX500Principal().getName();
      118                 LdapName ldapDN = new LdapName(dn);
      119                 String name = dn;
      120                 for (Rdn rdn : ldapDN.getRdns())
      121                 {
      122                     if (rdn.getType().equalsIgnoreCase("CN"))
      123                     {
      124                         name = String.valueOf(rdn.getValue());
      125                         break;
      126                     }
      127                 }
      128                 return name;
      129             }
      130             catch (InvalidNameException e)
      131             {
      132                 LOGGER.error("Error getting subject name from certificate");
      133                 return null;
      134             }
      
      

      We will highly appreciate your feedback!
       

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              OneisAll Anuhan Torgonshar
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated: