PARQUET-401: Deprecate Log and move to SLF4J Logger #319

lw-lin · 2016-02-02T09:38:03Z

The current Log class is intended to allow swapping out logger back-ends, but SLF4J already does this. It also doesn't expose as nice of an API as SLF4J, which can handle formatting to avoid the cost of building log messages that won't be used.

We should deprecate the org.apache.parquet.Log class and move to using SLF4J directly, instead of wrapping SLF4J (PARQUET-305).

liancheng · 2016-02-02T17:50:51Z

@proflin The last build failure was probably caused by Travis network issue. You may push a minor commit to trigger Travis again.

@rdblue Is there any way to trigger Travis without pushing a commit?

julienledem · 2016-02-02T23:43:54Z

@liancheng I think the owner of the PR can retrigger the build from the travis ui (need to be logged in)

julienledem · 2016-02-02T23:47:57Z

This seems fine.
can we have a perf test run to make sure this has no impact on perf?
I would think not but i'd rather be sure since the if (DEBUG) was optimized out at compile time.

This reverts commit a0d2f03.

This reverts commit dfb87aa.

This reverts commit 5df4d16.

This reverts commit 1430dae.

This reverts commit d14b3f8.

This reverts commit 0116fc2.

This reverts commit 5c77437.

This reverts commit 0610e8f.

…T-401--Deprecate-Log-and-move-to-SLF4J-Logger # Conflicts: # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ColumnChunkPageReadStore.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/InternalParquetRecordReader.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java

lw-lin · 2016-03-31T09:08:33Z

hi @rdblue, finally this is ready for a another (hopefully the last) round of review.

Changes since the first round:

many if (LOGGER.isXXXEnabled()) have been merged together;
some unnecessary if (LOGGER.isXXXEnabled()) have been removed;
the static final constants optimization has been applied to some more classes where necessary.

Also, things that are intentionally left out(let's fix them in following-up PRs):

moving to use the parametered form of LOGGER.xxx(), i.e., replacing Log.debug("msg is " + msg + ", number is " + number) with LOGGER.debug("msg is {}, number is {}", msg, number);
fixing the slf4j log level in the CI hadoop2 enviroment, i.e. slf4J binding leaked through Parquet's dependencies issue.

So, could you take another look at this? Thanks! :-)

…ate-Log-and-move-to-SLF4J-Logger # Conflicts: # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputForm at.java

lw-lin · 2016-04-19T03:58:40Z

two weeks' ping to some committer @rdblue :-)

rdblue · 2016-04-21T19:15:43Z

@lw-lin, thanks for working on this.

Looks like my previous comment about fixing the if (LOG.isDebugEnabled()) statements was misinterpreted. Sorry about that, I probably wasn't very clear.

You've replaced a lot of those calls with a static final constant at the top of the file. That's a bad thing because it means we can't turn on debug logging once the code is loaded. We should only do that when it matters for performance, which you said above was in MessageColumnIO, ColumnWriterV1, and ColumnWriterV2. The rest of the constants should be removed, and there shouldn't be unused constants like ERROR_ENABLED.

Most of the rest of the debug logging calls should not be wrapped by if (LOG.isDebugEnabled()) at all. Just call LOG.debug(...) and let the framework decide. The benefit of checking the log level is to avoid expensive operations to prepare a log message that is discarded at the current log level. Using the SLF4J formatting calls mostly avoids that.

The only time we should use LOG.isDebugEnabled() is when there's an expensive call that can't be handled inside the logger. An example is debugging the total memory used: we would inspect all of the write framework before calling into the logger, so guarding that with a level check is a good idea. But when you can simply pass arguments into the logger and the work is done there by calling toString on what you pass in, there's no need for the check.

I think getting the debug logging right is going to require going through the code and making sure each call makes sense, rather than transforming certain patterns with an IDE.

rdblue · 2016-04-21T19:18:04Z

parquet-column/src/main/java/org/apache/parquet/io/RecordConsumerLoggingWrapper.java

+      if (DEBUG_ENABLED) {
+        ++indent;
+      }
+      if (DEBUG_ENABLED) {


@lw-lin, it looks like there are still some if statements that can be combined. Please make sure you check through the code before the next round of review for these.

julienledem · 2016-08-15T22:15:03Z

@rdblue @lw-lin what's next for this PR?

lw-lin · 2016-08-16T02:52:06Z

@rdblue @julienledem sorry for the late response -- oh I somehow missed @rdblue 's kind comments.

I'll update this within this week. Thanks @rdblue @julienledem !

julienledem · 2016-08-16T03:06:49Z

thank you @lw-lin !

# Conflicts: # parquet-column/src/main/java/org/apache/parquet/column/values/boundedint/BitWriter.java # parquet-column/src/main/java/org/apache/parquet/column/values/boundedint/BoundedIntValuesReader.java # parquet-column/src/main/java/org/apache/parquet/column/values/boundedint/BoundedIntValuesWriter.java # parquet-encoding/src/test/java/org/apache/parquet/column/values/bitpacking/TestByteBitPacking.java # parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/InternalParquetRecordReader.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileReader.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetFileWriter.java # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputFormat.java

Fix 2

lw-lin · 2016-09-06T08:46:25Z

This has been updated a lot -- @julienledem @rdblue would you take a look at your convenience?

lw-lin · 2016-09-06T08:46:56Z

Plus, five rounds of PerfTest show that, comparing to the Log.java approach, the slf4j-simple approach:

on average takes 0.87% longer in writing 1,000,000 records
on average takes 1.31% longer in reading 1,000,000 records

For detailed results please refer to https://docs.google.com/spreadsheets/d/1FLwD71WFmkEfqDWyo2pe1vfkk7BI6bJKMOnAzGCl5XI/edit#gid=1865972057)

lw-lin · 2016-09-06T14:26:57Z

parquet-common/src/test/java/org/apache/parquet/TestLog.java

-import org.junit.Assert;
-import org.junit.Test;
-
-public class TestLog {


this class is not necessary any more

julienledem · 2016-10-26T16:15:51Z

Thank you @lw-lin for this work. Sorry this particular PR didn't go through.
Those patches that touch a lot of files are painful to maintain.
PR #369 implemented a similar changes and now has been merged.
Thank your for your contribution @lw-lin !

lw-lin · 2016-10-27T02:25:17Z

Thank you @julienledem . I like that PR too. Thank you also @rdblue @liancheng for the efforts you'd put into this!

lw-lin added 10 commits February 1, 2016 10:25

Merge remote-tracking branch 'refs/remotes/apache/master'

839b458

Merge branch 'master' of https://github.com/proflin/parquet-mr

bb4283a

Preview of replacing Log.java with slf4j

0610e8f

Remove the static import of BaseRecordReader

5c77437

03~10

a0d2f03

11~15

dfb87aa

16~20

5df4d16

21~25

1430dae

26~30

d14b3f8

31~35

0116fc2

lw-lin added 17 commits February 3, 2016 09:56

Revert "03~10"

b50426b

This reverts commit a0d2f03.

Revert "11~15"

c98b22b

This reverts commit dfb87aa.

Revert "16~20"

6640a7d

This reverts commit 5df4d16.

Revert "21~25"

f40c091

This reverts commit 1430dae.

Revert "26~30"

0133d05

This reverts commit d14b3f8.

Revert "31~35"

22e93a4

This reverts commit 0116fc2.

Revert "Remove the static import of BaseRecordReader"

f22c5ef

This reverts commit 5c77437.

Revert "Preview of replacing Log.java with slf4j"

0473687

This reverts commit 0610e8f.

Updates

da95567

replace Log.java with slf4j for module [parquet.common]

9b43f9a

replace Log.java with slf4j for module [parquet.common]

d22d1af

replace Log.java with slf4j for module [parquet-encoding]

9b152ff

replace Log.java with slf4j for module [parquet-column] part 1

cd2861f

replace Log.java with slf4j for module [parquet-column] part 2

6335d76

replace Log.java with slf4j for module [parquet-column] part 3

3a99a6b

replace Log.java with slf4j for module [parquet-column] part 4

af36699

replace Log.java with slf4j for module [parquet-column] part 5

7c54e90

lw-lin closed this Mar 8, 2016

lw-lin reopened this Mar 8, 2016

lw-lin added 5 commits March 11, 2016 16:50

Add a static final field to optimize out LOGGER.isXXXEnabled() call

a9f50b6

Replace Log with slf.Logger for DictionaryFilter

ade4f52

Add a static final field to optimize out LOGGER.isXXXEnabled() call

fa51e9c

Merge or remove some if (LOGGER.isXXXEnabled())

bf8ed47

lw-lin changed the title ~~Parquet-401: Deprecate Log and move to SLF4J Logger~~ PARQUET-401: Deprecate Log and move to SLF4J Logger Mar 11, 2016

lw-lin and others added 2 commits March 11, 2016 20:29

Fix a minor compilation issue

10d6e32

Add static final fields to optimize out LOGGER.isXXXEnabled() call

a718d67

Merge remote-tracking branch 'apache/master' into PARQUET-401--Deprec…

f4a1ea4

…ate-Log-and-move-to-SLF4J-Logger # Conflicts: # parquet-hadoop/src/main/java/org/apache/parquet/hadoop/ParquetOutputForm at.java

rdblue reviewed Apr 21, 2016
View reviewed changes

lw-lin added 3 commits August 24, 2016 12:09

Fix LOGGER

0cc0eff

Fix 2

Fix Log

199398c

lw-lin reviewed Sep 6, 2016
View reviewed changes

julienledem mentioned this pull request Oct 5, 2016

PARQUET-423: Replace old Log class with SLF4J Logging #369

Closed

lw-lin closed this Oct 27, 2016

PARQUET-401: Deprecate Log and move to SLF4J Logger #319

PARQUET-401: Deprecate Log and move to SLF4J Logger #319

Uh oh!

Conversation

lw-lin commented Feb 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liancheng commented Feb 2, 2016

Uh oh!

julienledem commented Feb 2, 2016

Uh oh!

julienledem commented Feb 2, 2016

Uh oh!

lw-lin commented Mar 31, 2016

Uh oh!

lw-lin commented Apr 19, 2016

Uh oh!

rdblue commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdblue Apr 21, 2016

Choose a reason for hiding this comment

Uh oh!

julienledem commented Aug 15, 2016

Uh oh!

lw-lin commented Aug 16, 2016

Uh oh!

julienledem commented Aug 16, 2016

Uh oh!

lw-lin commented Sep 6, 2016

Uh oh!

lw-lin commented Sep 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lw-lin Sep 6, 2016

Choose a reason for hiding this comment

Uh oh!

julienledem commented Oct 26, 2016

Uh oh!

lw-lin commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lw-lin commented Feb 2, 2016 •

edited

Loading

rdblue commented Apr 21, 2016 •

edited

Loading

lw-lin commented Sep 6, 2016 •

edited

Loading