[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc@117 PS5, Line 117: unlink(socket_path_.c_str()); > As an additional point for advocating for the change, I looked through test I agree thats true, but this is slightly different given this is specifically for socket usage. The precedent for this in the codebase is in the one place where unlink was already used in socket-test.cc -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 22 Sep 2020 16:07:05 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Fix Unix socket usage on macOS macOS does not support abstract namespaces for sockets. This patch fixes the socket usage by using a real path instead of abstract namespaces. Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Reviewed-on: http://gerrit.cloudera.org:8080/16458 Tested-by: Grant Henke Reviewed-by: Alexey Serbin --- M src/kudu/rpc/rpc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/net/socket-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 5 files changed, 41 insertions(+), 10 deletions(-) Approvals: Grant Henke: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Fix Unix socket usage on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 6: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc@117 PS5, Line 117: unlink(socket_path_.c_str()); > I am not sure. I think I prefer ulink given it the defacto way to handle so As an additional point for advocating for the change, I looked through tests and found that we primarily use Env::X() for filesystem operations instead of direct system calls. But I guess it's OK to use unlink() here since it's not critical in this context of a clean-up for a test scenario. So, feel free to ignore if my attempt to sell this change isn't appealing :) -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 21 Sep 2020 23:20:28 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has removed a vote on this change. Change subject: Fix Unix socket usage on macOS .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 6: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 21 Sep 2020 02:44:11 + Gerrit-HasComments: No
[kudu-CR] Fix Unix socket usage on macOS
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16458 to look at the new patch set (#6). Change subject: Fix Unix socket usage on macOS .. Fix Unix socket usage on macOS macOS does not support abstract namespaces for sockets. This patch fixes the socket usage by using a real path instead of abstract namespaces. Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb --- M src/kudu/rpc/rpc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/net/socket-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 5 files changed, 41 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16458/6 -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test-base.h@426 PS5, Line 426: virtual > nit: 'virtual' isn't necessary here -- this class overrides TearDown() decl Done http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc@117 PS5, Line 117: unlink(socket_path_.c_str()); > here and below: would env_->DeleteFile(socket_path_) be a better fit? At l I am not sure. I think I prefer ulink given it the defacto way to handle socket files and we don't check or care much about error handling. http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/util/net/socket-test.cc@162 PS5, Line 162: unlink(path.c_str()); > nit: maybe, replace with See my other response. -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 21 Sep 2020 01:47:34 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 5: (3 comments) Looks good overall, just a couple nits. http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test-base.h@426 PS5, Line 426: virtual nit: 'virtual' isn't necessary here -- this class overrides TearDown() declared somewhere in the base http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/rpc/rpc-test.cc@117 PS5, Line 117: unlink(socket_path_.c_str()); here and below: would env_->DeleteFile(socket_path_) be a better fit? At least, it provides a common path for logging (like WARN_IF_NOT_ON()) and error handling. http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/5/src/kudu/util/net/socket-test.cc@162 PS5, Line 162: unlink(path.c_str()); nit: maybe, replace with WARN_NOT_OK(env_->DeleteFile(path)); ? -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Sep 2020 23:08:48 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/16458/3/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/3/src/kudu/rpc/rpc-test.cc@116 PS3, Line 116: > That's cool! Done http://gerrit.cloudera.org:8080/#/c/16458/1/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/1/src/kudu/util/net/socket-test.cc@117 PS1, Line 117: unix: > nit: does it make sense to add a CHECK() based on string match in case of m Done -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Sep 2020 19:22:33 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16458 to look at the new patch set (#4). Change subject: Fix Unix socket usage on macOS .. Fix Unix socket usage on macOS macOS does not support abstract namespaces for sockets. This patch fixes the socket usage by using a real path instead of abstract namespaces. Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb --- M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/net/socket-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 6 files changed, 42 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16458/4 -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Fix Unix socket usage on macOS
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16458 ) Change subject: Fix Unix socket usage on macOS .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/16458/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16458/1//COMMIT_MSG@9 PS1, Line 9: macOS does not support abstract namespaces for sockets. > That impacts the tests that use ClientTestUnixSocket. client-test.cc Client Ah, it for some reason I was under impression that changes in server_base.cc are not necessary to make those tests in question to pass on macOS. I think the way how it's now in server_base.cc is totally fine. I was just advocating for having _tests_ using POSIX-compatible way of creating Unix domain sockets, unless they specifically target to test the anonymous namespace Unix sockets on Linux, like SocketTest.TestUnixSocketAbstractNamespace scenario does. http://gerrit.cloudera.org:8080/#/c/16458/3/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/3/src/kudu/rpc/rpc-test.cc@116 PS3, Line 116: GetTestSocketPath That's cool! However, it seems the corresponding files will accumulat under the test directory (which is /tmp/kudutest- by default). Is it possible to clean those files up automatically? http://gerrit.cloudera.org:8080/#/c/16458/1/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/16458/1/src/kudu/util/net/socket-test.cc@117 PS1, Line 117: > unix:@ followed by a bunch of null characters. nit: does it make sense to add a CHECK() based on string match in case of macOS as well? -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 16 Sep 2020 21:25:27 + Gerrit-HasComments: Yes
[kudu-CR] Fix Unix socket usage on macOS
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16458 to look at the new patch set (#3). Change subject: Fix Unix socket usage on macOS .. Fix Unix socket usage on macOS macOS does not support abstract namespaces for sockets. This patch fixes the socket usage by using a real path instead of abstract namespaces. Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb --- M src/kudu/rpc/rpc-test.cc M src/kudu/server/server_base.cc M src/kudu/util/net/socket-test.cc M src/kudu/util/test_util.cc M src/kudu/util/test_util.h 5 files changed, 29 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/16458/3 -- To view, visit http://gerrit.cloudera.org:8080/16458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I434e92e86fb87d1085c35b5d707d1238295192fb Gerrit-Change-Number: 16458 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120)
