[DRAFT] HIVE-29173: Replace string concatenation in log messages with logging format #6053
Conversation
|
abstractdog
changed the title
There was a problem hiding this comment.
The main concern is the logging that involves exceptions. In most cases the number of placeholders is different from the number of arguments so I am wondering if we get the expected output. Is the exception, which usually appears at the end, logged correctly?
| @@ -241,40 +241,34 @@ private void listBeans(JsonGenerator jg, ObjectName qry, String attribute, | |||
| } catch (AttributeNotFoundException e) { | |||
| // If the modelerType attribute was not found, the class name is used | |||
| // instead. | |||
| LOG.error("getting attribute " + prs + " of " + oname | |||
| + " threw an exception", e); | |||
| LOG.error("getting attribute {} of {} threw an exception", prs, oname, e); | |||
There was a problem hiding this comment.
I am wondering if the exception serialization remains the same after this change since we no longer use the API that accepts a Throwable.
There was a problem hiding this comment.
In addition it is strange that we use two placeholders {} but we pass three arguments.
| @@ -85,7 +85,7 @@ default Table loadHmsTable() throws TException, InterruptedException { | |||
| try { | |||
| return metaClients().run(client -> client.getTable(database(), table())); | |||
| } catch (NoSuchObjectException nte) { | |||
| LOG.trace("Table not found {}", database() + "." + table(), nte); | |||
| LOG.trace("Table not found {}.{}", database(), table(), nte); | |||
There was a problem hiding this comment.
Two placeholders but three arguments.
| @@ -104,11 +104,10 @@ public static void importCredentialsFromCurrentSubject(KuduClient client) { | |||
| // 'client'. This is necessary if we want to support a job which | |||
| // reads from one cluster and writes to another. | |||
| if (!tok.getService().equals(service)) { | |||
| LOG.debug("Not importing credentials for service " + service + | |||
| "(expecting service " + service + ")"); | |||
| LOG.debug("Not importing credentials for service {} (expecting service {})", service, service); | |||
There was a problem hiding this comment.
Passing the same object two times does not make much sense but I guess outside the scope of the PR.
| @@ -77,7 +77,7 @@ public synchronized void close() throws IOException { | |||
| try { | |||
| din.close(); | |||
| } catch (Exception err) { | |||
| LOG.error("Error closing input stream:" + err.getMessage(), err); | |||
| LOG.error("Error closing input stream: {}", err.getMessage(), err); | |||
There was a problem hiding this comment.
How about something simpler:
LOG.error("Error closing input stream:", err);| host = socketAddress.getHostName(); | ||
| } | ||
| } catch (UnknownHostException e) { | ||
| LOG.warn("Ignoring resolution issues for host: " + host, e); | ||
| LOG.warn("Ignoring resolution issues for host: {}", host, e); |
| @@ -391,8 +391,7 @@ private void cleanupLoggingRootDir() { | |||
| try { | |||
| FileUtils.forceDelete(operationLogRootDir); | |||
| } catch (Exception e) { | |||
| LOG.warn("Failed to cleanup root dir of HS2 logging: " + operationLogRootDir | |||
| .getAbsolutePath(), e); | |||
| LOG.warn("Failed to cleanup root dir of HS2 logging: {}", operationLogRootDir.getAbsolutePath(), e); | |||
| LOG.info("This instance of HiveServer2 has been removed from the list of server " | ||
| + "instances available for dynamic service discovery. " | ||
| + "The last client session has ended - will shutdown now."); | ||
| LOG.info("This instance of HiveServer2 has been removed from the list of server " |
| LOG.warn( | ||
| "Unable to inherit permissions for file " + target + " from file " + sourceStatus.getFileStatus().getPath(), | ||
| e.getMessage()); | ||
| LOG.warn("Unable to inherit permissions for file {} from file {}", |
| LOG.debug("The details are: " + e, e); | ||
| LOG.info("Skipping ACL inheritance: File system for path {} does not support ACLs " + | ||
| "but dfs.namenode.acls.enabled is set to true.", file); | ||
| LOG.debug("The details are: {}", e); |
There was a problem hiding this comment.
| LOG.debug("The details are: {}", e); | |
| LOG.debug("The details are:", e); |
| LOG.debug("The details are: " + e, e); | ||
| LOG.info("Skipping ACL inheritance: File system for path {} does not support ACLs " + | ||
| "but dfs.namenode.acls.enabled is set to true.", target); | ||
| LOG.debug("The details are: {}", e); |
There was a problem hiding this comment.
| LOG.debug("The details are: {}", e); | |
| LOG.debug("The details are:", e); |
|
@zabetak : in general, with SLF4J, the last argument is always treated in a special way, which can be a throwable, and doesn't have to be added to the logging format btw, this PR is getting bigger and bigger, that's why I set this to draft, not sure where it ends :D |
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?