You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/05/05 19:13:05 UTC

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15861

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................

IMPALA-9714: Fix edge cases in SimpleLogger and add test

SimpleLogger is used for several existing log types and a change
to use it for the data cache access trace is underway. Since this
is commonly used, it is useful to nail down specific semantics and
test them.

This fixes the following edge cases:
1. LoggingSupport::DeleteOldLogs() currently maintains a map from mtime
to the filename in order to decide which files need to be deleted.
This stops working when there are fast updates to the log, because
mtime has seconds resolution and DeleteOldLogs() is only able to
recognize a single file per mtime with the current map. This changes
the map to a set of pairs of mtime + filename. The behavior is
identical except that if there are multiple files with the same
mtime, they each get their own entry in the set. This allows
DeleteOldLogs() to more accurately maintain the maximum log files.
2. SimpleLogger::Init() now enforces the limit on the maximum number
of log files. This provides a clear semantic when dealing with
preexisting files from a previous incarnation of the same logger.
3. SimpleLogger will now create any intermediate directories when
creating the logging directory (i.e. existingdir/a/b/c works).

This also introduces SimpleLogger::GetLogFiles(), which is a static
function to get the log files given a directory and prefix. This
is necessary for testing, but it also will be useful for code that
wants to process logs from SimpleLogger.

Testing:
- Added a new simple-logger-test that codifies the expected behavior
- Ran core tests

Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
---
M be/src/util/CMakeLists.txt
M be/src/util/logging-support.cc
A be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
5 files changed, 350 insertions(+), 7 deletions(-)

git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/15861/2
--
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 3: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 May 2020 02:12:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2:

(3 comments)

I addressed some comments and then also moved the enforcement of max_audit_event_log_files to use the enforcement from SimpleLogger rather than its own code.

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.h
File be/src/util/simple-logger.h:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.h@60
PS2, Line 60:   bool initialized_ = false;
> nit: docs
Done


http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc
File be/src/util/simple-logger.cc:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@31
PS2, Line 31: #include "common/names.h"
> nit: why the include ordering change?
common/names.h is special in that it only is defining aliases, and it looks at which compile guards are defined to determine which aliases to add. Best practice is for it to go last so that all the compile guards from the include files are defined.
https://github.com/apache/impala/blob/master/be/src/common/names.h#L28-L29

In this case, nothing changes, and this isn't really needed for this change.


http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@141
PS2, Line 141:   FileSystemUtil::Directory dir(log_dir);
> The difference in pattern I think is probably a good enough reason to leave
I'm going to leave this as-is for this change. I will file a followup JIRA. I got it partially working, but I would need more tests for the glog tests. I moved the regex filtering to FileSystemUtil::GetEntryNames() and simplified this function to use it. If we do the further change, it won't be that big.



-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 May 2020 23:52:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 May 2020 21:43:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 May 2020 20:09:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5764/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 May 2020 16:29:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc
File be/src/util/simple-logger.cc:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@141
PS2, Line 141:   FileSystemUtil::Directory dir(log_dir);
It seems like the logic here is basically duplicating the logic in the first part of DeleteOldLogs where we iterate over all of the files matching a pattern and sort them by time, except this is better implemented.

Is there a reason to keep both versions of the logic around?

I guess maybe the point is that we can guarantee here that the log filenames have timestamps, since they come from GenerateLogFileName() whereas DeleteOldLogs() doesn't technically require that, though I think it might be true at all of its call sites, and with this change its sort of implicitly relying on it for the sorting anyways, so maybe we just want to make that requirement explicit.



-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 May 2020 17:52:59 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.

Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2:

(1 comment)

Not your change, but I happened to notice that the audit logs use SimpleLogger but for some reason don't use its built in log rotation functionality and instead gets rotated on the same thread as the glog files.

I couldn't figure out from 'git blame' if this was intentional, but it seems wrong so we might want to go ahead and fix it.

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc
File be/src/util/simple-logger.cc:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@141
PS2, Line 141:   FileSystemUtil::Directory dir(log_dir);
> I see what you mean. I think we would be able to switch to using a delete f
The difference in pattern I think is probably a good enough reason to leave this as-is for now if you prefer.



-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 May 2020 17:11:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................

IMPALA-9714: Fix edge cases in SimpleLogger and add test

Testing:
- Added a new simple-logger-test that codifies the expected behavior
- Ran core tests

Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Reviewed-on: http://gerrit.cloudera.org:8080/15861
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/init.cc
M be/src/common/logging.cc
M be/src/common/logging.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/filesystem-util-test.cc
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
M be/src/util/logging-support.cc
A be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
13 files changed, 419 insertions(+), 49 deletions(-)

Approvals:
Thomas Tauber-Marshall: Looks good to me, approved
Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc
File be/src/util/simple-logger.cc:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@141
PS2, Line 141:   FileSystemUtil::Directory dir(log_dir);
> It seems like the logic here is basically duplicating the logic in the firs
I see what you mean. I think we would be able to switch to using a delete function based on filename sorting. For the SimpleLogger users, it would work. DeleteOldLogs() is also used for glog filenames, which have a form like:
impalad.${hostname}.${user}.log.${severity}.${date}-${time}.${pid}
It would require a different regex, but filename sort is probably reasonable. Given that we only really expect one impalad per node, the pid is not important. Using filename sort would avoid all the stat() calls to get mtime.

I will take a look at this. If it involves any complication, I may want to split this off into a separate JIRA.



-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sat, 09 May 2020 00:03:18 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/6032/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 May 2020 00:45:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5813/ DRY_RUN=true


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 May 2020 20:58:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.

Hello Thomas Tauber-Marshall, Sahil Takiar, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15861

to look at the new patch set (#3).

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................

IMPALA-9714: Fix edge cases in SimpleLogger and add test

SimpleLogger is used for several existing log types and a change
to use it for the data cache access trace is underway. Since this
is commonly used, it is useful to nail down specific semantics and
test them.

This fixes the following edge cases:
1. LoggingSupport::DeleteOldLogs() currently maintains a map from mtime
   to the filename in order to decide which files need to be deleted.
   This stops working when there are fast updates to the log, because
   mtime has seconds resolution and DeleteOldLogs() is only able to
   recognize a single file per mtime with the current map. This changes
   the map to a set of pairs of mtime + filename. The behavior is
   identical except that if there are multiple files with the same
   mtime, they each get their own entry in the set. This allows
   DeleteOldLogs() to more accurately maintain the maximum log files.
2. SimpleLogger::Init() now enforces the limit on the maximum number
   of log files. This provides a clear semantic when dealing with
   preexisting files from a previous incarnation of the same logger.
3. SimpleLogger will now create any intermediate directories when
   creating the logging directory (i.e. existingdir/a/b/c works).
4. This changes the enforcement moves enforcement max_audit_event_log_files
   to use the limits provided by SimpleLogger rather than a background
   thread calling DeleteOldLogs() periodically.

This also introduces SimpleLogger::GetLogFiles(), which is a static
function to get the log files given a directory and prefix. This
is necessary for testing, but it also will be useful for code that
wants to process logs from SimpleLogger.

Testing:
 - Added a new simple-logger-test that codifies the expected behavior
 - Ran core tests

Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
---
M be/src/common/init.cc
M be/src/common/logging.cc
M be/src/common/logging.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/CMakeLists.txt
M be/src/util/filesystem-util-test.cc
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
M be/src/util/logging-support.cc
A be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
13 files changed, 419 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/15861/3
-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2: -Verified

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/5965/ : Initial code review checks failed. See linked job for details on the failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 05 May 2020 19:47:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9714: Fix edge cases in SimpleLogger and add test

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.

Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15861 )

Change subject: IMPALA-9714: Fix edge cases in SimpleLogger and add test
......................................................................


Patch Set 2: Code-Review+1

(2 comments)

some nits, overall LGTM pending Thomas' comment

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.h
File be/src/util/simple-logger.h:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.h@60
PS2, Line 60:   bool initialized_ = false;
nit: docs


http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc
File be/src/util/simple-logger.cc:

http://gerrit.cloudera.org:8080/#/c/15861/2/be/src/util/simple-logger.cc@31
PS2, Line 31: #include "common/names.h"
nit: why the include ordering change?



-- 
To view, visit http://gerrit.cloudera.org:8080/15861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idd092a65b31d34f40a660cab7b5e0695a3627c78
Gerrit-Change-Number: 15861
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 11 May 2020 16:17:26 +0000
Gerrit-HasComments: Yes