[
https://issues.apache.org/jira/browse/QPID-8291?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Anuhan Torgonshar updated QPID-8291:
------------------------------------
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
Unknown macro: \{ 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
Unknown macro: \{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
Unknown macro: \{217 if(dcStr == null)218 { 219 dcStr =
rdn.getValue().toString(); 220 }221 else222 \{ 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 }
Look forward to your review and feedback!
was:
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 }
Look forward to your review and feedback!
> Inconsistent log level practices in source code
> -----------------------------------------------
>
> Key: QPID-8291
> URL: https://issues.apache.org/jira/browse/QPID-8291
> Project: Qpid
> Issue Type: Improvement
> Components: Broker-J
> Affects Versions: qpid-java-6.1.7
> Reporter: Anuhan Torgonshar
> Priority: Major
> Labels: easyfix
>
> 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
> Unknown macro: \{ 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
> Unknown macro: \{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
> Unknown macro: \{217 if(dcStr == null)218 { 219 dcStr =
> rdn.getValue().toString(); 220 }221 else222 \{ 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 }
> Look forward to your review and feedback!
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
