Andrew Sherman has posted comments on this change. (
http://gerrit.cloudera.org:8080/14531 )
Change subject: IMPALA-8065 Add OS version and Kernel version in OSInfo
..
Patch Set 3:
(8 comments)
Code looks good, Comments and commit message need more work, I think
http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG@8
PS3, Line 8:
I think it is important to write a commit message that can be understood
by someone who has not read the code you are changing. Sometimes it
helps to explain what the code is.
OsInfo is used to get human readable information about the OS on which
Impala is running.
Before this change OsInfo::DebugString() would print two lines:
- OS version: the long name of the Linux kernel from /proc/version
- Clock: the type of clock used
After this change OsInfo::DebugString() will print three lines:
- OS version: the short name of the Linux distribution.
If Docker is being used this is the name of the Container OS
- Kernel version: the long name of the Linux kernel from /proc/version
If Docker is being used this is the description of the Host OS.
- Clock: the type of clock used
http://gerrit.cloudera.org:8080/#/c/14531/3//COMMIT_MSG@15
PS3, Line 15: OS version: "Ubuntu 16.04.6 LTS"
Why does OS Version have quotes but Kernel version does not?
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt
File be/src/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt@124
PS3, Line 124: os-info-test.cc
These should be in alphabetical order
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/CMakeLists.txt@187
PS3, Line 187: ADD_UNIFIED_BE_LSAN_TEST(os-info-test "OsInfo.*")
These should be in alphabetical order
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info-test.cc
File be/src/util/os-info-test.cc:
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info-test.cc@25
PS3, Line 25: ASSERT_NE(osinfo.os_version(), "Unknown");
Add a simple comment explaining the test
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h
File be/src/util/os-info.h:
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h@35
PS3, Line 35: /// Name of the OS release.
Simple name of the OS.
If Docker is used this is the name of the Container OS
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.h@42
PS3, Line 42: /// and the version of a GCC compiler used to build it.
The version of Linux kernel and the version of the compiler used to build it.
If Docker is used this is the Host OS.
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.cc
File be/src/util/os-info.cc:
http://gerrit.cloudera.org:8080/#/c/14531/3/be/src/util/os-info.cc@62
PS3, Line 62: os_path = "/etc/centos-release";
Add a comment saying "Only old distributions like Centos 6"
--
To view, visit http://gerrit.cloudera.org:8080/14531
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I848c9e53ee4e0bf8ae0874bb6da28e8efa7f7c8a
Gerrit-Change-Number: 14531
Gerrit-PatchSet: 3
Gerrit-Owner: Xiaomeng Zhang
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Xiaomeng Zhang
Gerrit-Comment-Date: Thu, 31 Oct 2019 00:06:28 +
Gerrit-HasComments: Yes
