Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/14531 )
Change subject: IMPALA-8065 OSInfo produces somewhat misleading output when running in container ...................................................................... Patch Set 2: (7 comments) Looks like this is getting close. http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-8065 OSInfo produces somewhat misleading output when running in container Sometimes it is better to have the first line be not the bug description but a oneline summary of the change. http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@9 PS2, Line 9: Original we get /proc/version dispalyed as OS version, while it's Nit: displayed http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@14 PS2, Line 14: Tested locally, the displayed OS Info in localhost:25020 is: "in localhost:25020" is confusing, maybe say on a Ubuntu 16 dev box? http://gerrit.cloudera.org:8080/#/c/14531/2//COMMIT_MSG@21 PS2, Line 21: debian. Each OS picked one version to test. Can you add a unit test where you check that os version and kernel version are not equal to "Unknown", then you will know this works on all tested platforms, and that it does not break on future platforms. http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h File be/src/util/os-info.h: http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h@35 PS2, Line 35: static const std::string os_version() { Add a description of what this is http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.h@40 PS2, Line 40: static const std::string kernel_version() { Add a description of what this is http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.cc File be/src/util/os-info.cc: http://gerrit.cloudera.org:8080/#/c/14531/2/be/src/util/os-info.cc@58 PS2, Line 58: string os_path_ = "Unknown"; local variables should not have the "_" suffux -- 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: 2 Gerrit-Owner: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Xiaomeng Zhang <xiaom...@cloudera.com> Gerrit-Comment-Date: Tue, 29 Oct 2019 23:25:51 +0000 Gerrit-HasComments: Yes
