Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/19251 )
Change subject: IMPALA-11730: Add support for spilling to Ozone ...................................................................... Patch Set 10: (2 comments) Overall, this makes sense, just a couple minor things http://gerrit.cloudera.org:8080/#/c/19251/10/be/src/runtime/bufferpool/buffer-pool-test.cc File be/src/runtime/bufferpool/buffer-pool-test.cc: http://gerrit.cloudera.org:8080/#/c/19251/10/be/src/runtime/bufferpool/buffer-pool-test.cc@124 PS10, Line 124: string GetDefaultFsPath(string path) { : const char* filesystem_prefix = getenv("FILESYSTEM_PREFIX"); : if (filesystem_prefix != nullptr && filesystem_prefix[0] != '\0') { : return Substitute("$0$1", filesystem_prefix, path); : } : return Substitute("$0$1", test_env_->exec_env()->default_fs(), path); : } Since we use this code multiple times, can we put it in a central place like util/hdfs-util.h or a new header in testutil? http://gerrit.cloudera.org:8080/#/c/19251/10/be/src/runtime/bufferpool/buffer-pool-test.cc@132 PS10, Line 132: string REMOTE_URL; Nit: It looks like this is only used internally. Can we make it protected or private? (Not critical, because these are just unit tests) -- To view, visit http://gerrit.cloudera.org:8080/19251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5837c30357363f727ca832fb94169f2474fb4f6f Gerrit-Change-Number: 19251 Gerrit-PatchSet: 10 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 06 Dec 2022 19:49:49 +0000 Gerrit-HasComments: Yes
