[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?
| // 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.
| 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.
| 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.
| 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);| } | ||
| } catch (UnknownHostException e) { | ||
| LOG.warn("Ignoring resolution issues for host: " + host, e); | ||
| LOG.warn("Ignoring resolution issues for host: {}", host, e); |
| } 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 |
|
It looks like a tiring and boring manual work. What about giving a shot to an automatized solution, like OpenRewrite? |
4 participants
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?