[ https://issues.apache.org/jira/browse/SOLR-4547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13636999#comment-13636999 ]
Hoss Man commented on SOLR-4547: -------------------------------- Shawn: my previous comment was based on the mistaken impression that these log messages were leveraging the toString() of the IndexCommit and List<IndexCommit> objects ... looking at your patch, i realize now that these ugly "str()" methods were doing the work. sorry for the confusion specific concerns i have about your patch... 1) in the case of logging a List<IndexCommit>, the existing code does a single INFO message with the details of each commit, but in your patch each commit gets it's own log message (x2, INFO and DEBUG) because of how you've got logCommits() looping and delegating to logCommit(). This seems like a bad idea since in the case of multiple SolrCores these can easily get interleaved and hard to coalesce. 2) both the existing str functions and your new logCommit/logCommits functions suffer from the problem of doing a lot of String building even in the case where hte logging level is so high the messages will be ignored. 3) i'm not a fan of the the dual INFO/DEBUG logging going on inside of logCommit/logCommits ... i can't really explain it well, but it strikes me as being too "hidden" from the flow of the methods that call it, i'd rather see the log.info and log.debug calls inside each method where it happens so it's more obvious we are infact logging two different ways there. My suggestion for dealing with this would probably be... a) introduce some private static inner classes that are really cheap to construct and can wrap a List<IndexCommit> and have toString() methods that render out all the details we want b) use these new cheap classes directly in info & debug calls that leverage placeholder markers something along the lines of (not fully thought through)... {noformat} log.info("SolrDeletionPolicy.onCommit: commits: {}", new CommitsLoggingInfo(commits)); log.info("SolrDeletionPolicy.onCommit: commits: {}", new CommitsLoggingDebug(commits)); ... private static class CommitsLoggingInfo { ... public final String toString() { StringBuilder sb; ... for (IndexCommit c : commits) { appendDetails(sb, c)); } ... return sb.toString(); } protected void appendDetails(StringBuilder s, IndexCommit c) { // add summary details, ie: not all the freaking files. } } private static class CommitsLoggingDebug extends CommitsLoggingInfo { ... protected void appendDetails(StringBuilder s, IndexCommit c) { super.appendDetails(s, c); s.append(c.getFileNames()); } } {noformat} (of course, another way to go would be to sprinkly a bunch of "if (log.isDebugEnabled())" and "if (log.isInfoEnabled())" arround your existing patch, but that would still leave concern #1) > Commit logging - move index filenames to DEBUG. > ----------------------------------------------- > > Key: SOLR-4547 > URL: https://issues.apache.org/jira/browse/SOLR-4547 > Project: Solr > Issue Type: Improvement > Components: update > Affects Versions: 4.1 > Reporter: Shawn Heisey > Fix For: 4.3, 5.0 > > Attachments: solr-4547-insanity.txt, SOLR-4547.patch > > > When an index commit is done (at least with MMap/NRT, haven't checked other > Directory implementations), all of the filenames in the index are logged > three times at INFO. I think the information has value to someone if they > are debugging, but not to most users. > I propose that we move this part of the log to DEBUG and provide text that > allows the person/program viewing the log to link the smaller INFO log entry > with the new one, probably the directory path. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org