DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default #930

vvysotskyi · 2017-09-01T10:22:46Z

No description provided.

arina-ielchiieva · 2017-09-01T10:25:42Z

LGTM. +1

jinfengni · 2017-09-01T17:38:43Z

I have some concern about this PR.

AFASIK, some of Drill developers use Lilith to debug issues when running individual unit testcases. If remove it, does it mean people has to manually reverse this PR, in order to get log in Lilith?

If you shutdown Lilith application while running unit test suite, does it hang as well?

vvysotskyi · 2017-09-01T17:49:46Z

Unit tests hang when Lilith application is not running, but when ClassicMultiplexSocketAppender in logback.xml is defined and used in the loggers.

priteshm · 2017-09-01T17:54:55Z

@vvysotskyi is it possible to change the port number for Lilith to avoid the conflict?

vvysotskyi · 2017-09-01T18:00:02Z

@priteshm, as I have mentioned in the Jira DRILL-5761, port in Lilith UI cannot be changed (huxi/lilith#10).

jinfengni · 2017-09-01T18:05:21Z

Per this hive doc, seems it's possible to change hiveserver2 port #.

Can you try to change hivesever2# if the other one is hardcoded?

https://cwiki.apache.org/confluence/display/Hive/Setting+Up+HiveServer2

vvysotskyi · 2017-09-01T18:26:33Z

Yes, we can change hiveserver2 port #, but I think that it would be better to disable Lilith by default since hiveserver2 port number may be used in the configs of other applications and it would be cumbersome to change all that configs.

jinfengni · 2017-09-01T18:35:01Z

We have run unit in our test clusters and I never heard that people complained that unit tests failed due to Lilith configuration.

vvysotskyi · 2017-09-04T13:25:09Z

I have implemented the proposal of @vrozov to conditionally enable Lilith appender. I have used ThresholdFilter in Lilith appender, so it was not needed to change all places where it is used.
An example of how to enable Lilith was added to the Jira description.
@jinfengni, @vrozov could you please take a look?

vrozov

As other modules have the dependency on drill-common-tests.jar in the test scope, common/src/test/resources/logback.xml is already in the classpath for other modules, will be it better to unify logback.xml in a single place?

vrozov · 2017-09-04T14:40:34Z

common/src/test/resources/logback.xml

@@ -16,17 +16,22 @@
    <ReconnectionDelay>10000</ReconnectionDelay>
    <IncludeCallerData>true</IncludeCallerData>
    <RemoteHosts>${LILITH_HOSTNAME:-localhost}</RemoteHosts>
+    <!-- Disables Lilith ClassicMultiplexSocketAppender since its use causes hanging


My guess is that using filter does not disable the appender and it is enabled but does not produce the output. There is still a question whether the appender tries to connect to the port and what is the behavior in case the connection can't be established (does it constantly retry to re-establish connection every 10 sec) or can be established but to HiveServer2 instead of Lilith server.

I agree with you that it does not disable Lilith. The connection is established with HiveServer2 instead of Lilith server and unit tests hang when MultiplexSendBytesService calls sendBytes() method.
I have updated changes, so now Lilith is disabled.

jinfengni · 2017-09-05T05:30:34Z

The proposal from @vrozov makes sense to me. As long as Lilith is not completely disabled, I'm fine with it.

vvysotskyi

I have unified logback config file in a single place for tests and updated changes. @vrozov, could you please take a look?

vvysotskyi · 2017-09-06T12:29:32Z

common/src/test/resources/logback.xml

@@ -16,17 +16,22 @@
    <ReconnectionDelay>10000</ReconnectionDelay>
    <IncludeCallerData>true</IncludeCallerData>
    <RemoteHosts>${LILITH_HOSTNAME:-localhost}</RemoteHosts>
+    <!-- Disables Lilith ClassicMultiplexSocketAppender since its use causes hanging


I agree with you that it does not disable Lilith. The connection is established with HiveServer2 instead of Lilith server and unit tests hang when MultiplexSendBytesService calls sendBytes() method.
I have updated changes, so now Lilith is disabled.

paul-rogers

Very good improvement overall. Some questions about the default test log levels.

paul-rogers · 2017-09-10T00:52:42Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.drill" additivity="false">
+    <level value="debug"/>


Please put the test debug level at ERROR. I have a copy of logback-test.xml that I copy into each working branch that sets the level to ERROR. Then, specific tests set more detailed logging as needed. Otherwise, the console is bombarded with unwanted logging messages making it very hard to find those of interest.

This logger contains only Lilith appender, and since we are using Lilith only for debugging separate tests, I think it would be better to left logging level here at DEBUG. In this case, everyone who uses Lilith won't need to change logback file. Console appender is used in the root logger and it has ERROR logging level.

Can this logger be defined between line 21 and 22 in a single if condition?

Without comment with file appender yes, it can. Thanks, done.

paul-rogers · 2017-09-10T00:54:30Z

common/src/test/resources/logback-test.xml

+<!--    <appender-ref ref="FILE" /> -->
+  </logger>
+
+  <logger name="com.mapr" additivity="false">


Should this be in Apache Drill? Only the MapR profile pulls in MapR code... It is not clear that tests that happen to use the MapR profile want debug level logging from this subsystem. Better to use the LogFixture to set more detailed logging in those tests that need it.

Initially, I assumed that it would be useful for those, who work with the mapr-format-plugin. I agree with you that we should delete this logger.

paul-rogers · 2017-09-10T00:56:44Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.hadoop" additivity="false">
+    <level value="info"/>


See comment below about MapR. It is not clear that we always want info-level logging from Hadoop. Seems this should be enabled only in tests that want that detail to avoid cluttering output with unwanted messages.

Thanks for pointing this, already removed.

vvysotskyi

@paul-rogers, thanks for looking into this pull request, I have addressed all CR comments. Could you please take a look?

vvysotskyi · 2017-09-11T15:10:33Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.drill" additivity="false">
+    <level value="debug"/>


This logger contains only Lilith appender, and since we are using Lilith only for debugging separate tests, I think it would be better to left logging level here at DEBUG. In this case, everyone who uses Lilith won't need to change logback file. Console appender is used in the root logger and it has ERROR logging level.

vvysotskyi · 2017-09-11T15:17:18Z

common/src/test/resources/logback-test.xml

+<!--    <appender-ref ref="FILE" /> -->
+  </logger>
+
+  <logger name="com.mapr" additivity="false">


Initially, I assumed that it would be useful for those, who work with the mapr-format-plugin. I agree with you that we should delete this logger.

vvysotskyi · 2017-09-11T15:18:43Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.hadoop" additivity="false">
+    <level value="info"/>


Thanks for pointing this, already removed.

paul-rogers

Thanks for the improvement!
LGTM
+1

paul-rogers · 2017-09-11T21:45:03Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.drill" additivity="false">
+    <level value="debug"/>


vrozov · 2017-09-12T16:59:21Z

common/src/test/resources/logback-test.xml

+    </encoder>
+  </appender>
+
+  <!--


Is it necessary to have RollingFileAppender defined for unit test, especially that it is commented anyway?

No, it is not necessary. It's hard to predict which appenders people use and since RollingFileAppender makes query execution slower, I decided to add it, but just in the comment.
Thanks for pointing this, I removed this comment with the appender.

+1 I think that developers are unlikely to use rolling file appender and can redirect console to a file if necessary.

Unify logback files. closes apache#930

vvysotskyi

@vrozov, I have addressed all CR comments. Could you please take a look?

vvysotskyi · 2017-09-13T13:32:43Z

common/src/test/resources/logback-test.xml

+    </encoder>
+  </appender>
+
+  <!--


No, it is not necessary. It's hard to predict which appenders people use and since RollingFileAppender makes query execution slower, I decided to add it, but just in the comment.
Thanks for pointing this, I removed this comment with the appender.

vvysotskyi · 2017-09-13T13:43:03Z

common/src/test/resources/logback-test.xml

+  -->
+
+  <logger name="org.apache.drill" additivity="false">
+    <level value="debug"/>


Without comment with file appender yes, it can. Thanks, done.

paul-rogers · 2017-09-13T16:24:25Z

Holding of on the merge waiting for review by @vrozov.

vrozov

LGTM

vvysotskyi · 2017-09-13T20:25:25Z

@paul-rogers, @vrozov, thanks for the review. Squashed commits and rebased onto master.

Unify logback files.

Unify logback files. closes apache#930

vvysotskyi force-pushed the DRILL-5761 branch from be17b28 to 0368a64 Compare September 4, 2017 12:58

vrozov reviewed Sep 4, 2017

View reviewed changes

vvysotskyi commented Sep 6, 2017

View reviewed changes

vvysotskyi force-pushed the DRILL-5761 branch from 0368a64 to ca04b15 Compare September 6, 2017 12:37

paul-rogers requested changes Sep 10, 2017

View reviewed changes

vvysotskyi commented Sep 11, 2017

View reviewed changes

vvysotskyi force-pushed the DRILL-5761 branch from ca04b15 to b7e9ebf Compare September 11, 2017 16:02

paul-rogers approved these changes Sep 11, 2017

View reviewed changes

vrozov reviewed Sep 12, 2017

View reviewed changes

paul-rogers pushed a commit to paul-rogers/drill that referenced this pull request Sep 12, 2017

DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default.

386f9dc

Unify logback files. closes apache#930

vvysotskyi commented Sep 13, 2017

View reviewed changes

vvysotskyi force-pushed the DRILL-5761 branch from b7e9ebf to d9f1aaf Compare September 13, 2017 14:14

vrozov approved these changes Sep 13, 2017

View reviewed changes

vvysotskyi force-pushed the DRILL-5761 branch from d9f1aaf to 602d62f Compare September 13, 2017 20:20

DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default.

602d62f

Unify logback files.

paul-rogers pushed a commit to paul-rogers/drill that referenced this pull request Sep 14, 2017

DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default.

80e9f18

Unify logback files. closes apache#930

vvysotskyi closed this Sep 17, 2017

DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default #930

DRILL-5761: Disable Lilith ClassicMultiplexSocketAppender by default #930

Conversation

vvysotskyi commented Sep 1, 2017

arina-ielchiieva commented Sep 1, 2017

jinfengni commented Sep 1, 2017

vvysotskyi commented Sep 1, 2017

priteshm commented Sep 1, 2017

vvysotskyi commented Sep 1, 2017

jinfengni commented Sep 1, 2017

vvysotskyi commented Sep 1, 2017

jinfengni commented Sep 1, 2017 • edited Loading

vvysotskyi commented Sep 4, 2017

vrozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinfengni commented Sep 5, 2017

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-rogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-rogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvysotskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-rogers commented Sep 13, 2017

vrozov left a comment

Choose a reason for hiding this comment

vvysotskyi commented Sep 13, 2017

jinfengni commented Sep 1, 2017 •

edited

Loading