You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/09/22 16:32:25 UTC
[GitHub] [solr] markrmiller opened a new pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
markrmiller opened a new pull request #301:
URL: https://github.com/apache/solr/pull/301
https://issues.apache.org/jira/browse/SOLR-15640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller merged pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
Posted by GitBox <gi...@apache.org>.
markrmiller merged pull request #301:
URL: https://github.com/apache/solr/pull/301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller merged pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
Posted by GitBox <gi...@apache.org>.
markrmiller merged pull request #301:
URL: https://github.com/apache/solr/pull/301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #301:
URL: https://github.com/apache/solr/pull/301#discussion_r718874914
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
##########
@@ -64,17 +64,6 @@
@ThreadLeakLingering(linger = 10000)
public class SolrTestCase extends LuceneTestCase {
- /**
- * <b>DO NOT REMOVE THIS LOGGER</b>
- * <p>
- * For reasons that aren't 100% obvious, the existence of this logger is neccessary to ensure
- * that the logging framework is properly initialized (even if concrete subclasses do not
- * themselves initialize any loggers) so that the async logger threads can be properly shutdown
- * on completion of the test suite
- * </p>
- * @see <a href="https://issues.apache.org/jira/browse/SOLR-14247">SOLR-14247</a>
- * @see #shutdownLogger
- */
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review comment:
we could also remove the logger too then, right? And check that it doesn't fail in the way Hoss mentioned on SOLR-14247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] markrmiller commented on a change in pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #301:
URL: https://github.com/apache/solr/pull/301#discussion_r721881065
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
##########
@@ -64,17 +64,6 @@
@ThreadLeakLingering(linger = 10000)
public class SolrTestCase extends LuceneTestCase {
- /**
- * <b>DO NOT REMOVE THIS LOGGER</b>
- * <p>
- * For reasons that aren't 100% obvious, the existence of this logger is neccessary to ensure
- * that the logging framework is properly initialized (even if concrete subclasses do not
- * themselves initialize any loggers) so that the async logger threads can be properly shutdown
- * on completion of the test suite
- * </p>
- * @see <a href="https://issues.apache.org/jira/browse/SOLR-14247">SOLR-14247</a>
- * @see #shutdownLogger
- */
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review comment:
I don't see any reason to remove it. There are other things in what was the formerly the base class that should be here, there are plenty of things that will be here, no benifit to removing it - nice to have it there for someone to use vs not have a logger and have someone decide not to log something from the base test class because they would rather not go copy paste one in. I find it odd anyone wanted to remove it to begin with, of all the classes you expect you might not want to have a logger, the base test class does not seem like one of them.
Hoss hit that issue because that test class does not have a logger either and blah blah, if no one was interested in what was happening then, doubt there is any more interest now.
Anyway, the logger is used, so to remove it, you would have to remove the current logging. And then you could look into that test, but regardless of if you put this change in or not you would still not find hossmans fail, so it would all look good even if this change did nothing. This base class has slowly limped into almost not being a subtle trap. So there were actually multiple ways out of hoss's brain teaser, both useful fixes of broken stuff that affected other things and places as well.
There is a claim, you still want this, hoss's mystery thread leaks or not, something about overlapping logging output in tests if you want to believe them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #301: SOLR-15640: Flush / Shutdown Async Loggers and LogManager after a test.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #301:
URL: https://github.com/apache/solr/pull/301#discussion_r718874914
##########
File path: solr/test-framework/src/java/org/apache/solr/SolrTestCase.java
##########
@@ -64,17 +64,6 @@
@ThreadLeakLingering(linger = 10000)
public class SolrTestCase extends LuceneTestCase {
- /**
- * <b>DO NOT REMOVE THIS LOGGER</b>
- * <p>
- * For reasons that aren't 100% obvious, the existence of this logger is neccessary to ensure
- * that the logging framework is properly initialized (even if concrete subclasses do not
- * themselves initialize any loggers) so that the async logger threads can be properly shutdown
- * on completion of the test suite
- * </p>
- * @see <a href="https://issues.apache.org/jira/browse/SOLR-14247">SOLR-14247</a>
- * @see #shutdownLogger
- */
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review comment:
we could also remove the logger too then, right? And check that it doesn't fail in the way Hoss mentioned on SOLR-14247
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org