Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/19479 )
Change subject: [Tool] Limit tablets copying speed ...................................................................... Patch Set 1: (8 comments) Thank you for adding this. It would really help if you could add a little more details about the following: 1. I understand that you have mentioned about other services getting impacted due to network jam caused by tablet copy operation, but that doesn't explain why tablet copy operation would become a preferred bandwidth user over other services. Is there an existing case where this kind of behaviour was seen and what was making linux kernel to do that? 2. Adding detailed information (in commit message) about the new library APIs would be quite helpful for posterity. 3. Limiting the network speed might cause frequent context switching. Do we have any data on that from the performance point of view? http://gerrit.cloudera.org:8080/#/c/19479/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19479/1//COMMIT_MSG@9 PS1, Line 9: will nit: does http://gerrit.cloudera.org:8080/#/c/19479/1//COMMIT_MSG@11 PS1, Line 11: every nit: very http://gerrit.cloudera.org:8080/#/c/19479/1//COMMIT_MSG@15 PS1, Line 15: One host owns one token bucket. Do you want to explain it a little further. In the sense, what would owning token mean in context of throttling? http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tools/kudu-tool-test.cc@8871 PS1, Line 8871: "-enable_network_speed_limit=true -limit_network_speed=1000000000", Instead of modifying existing test case, could you please add a new one and use new flags over there. That way, both cases will be covered - one where user specifies these flags and other where old behaviour holds. http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tools/tool_action_local_replica.cc@686 PS1, Line 686: FLAGS_limit_network_speed <= 0) What if this value to too high? Would it make sense to put a ceiling limit on this value or we will just allow it as big a value as int64 can hold? http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tools/tool_action_local_replica.cc@686 PS1, Line 686: if (FLAGS_enable_network_speed_limit && FLAGS_limit_network_speed <= 0) { A similar check can be added to ensure either both flags are set or neither of them are. http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tserver/tablet_copy_client.cc@83 PS1, Line 83: limit_network_speed, 0, Either set this to some legit value (e.g. 100 MB/s, 1000 MB/s, etc) or make sure that user is enforced to set this value if enable_network_speed_limit is set true. http://gerrit.cloudera.org:8080/#/c/19479/1/src/kudu/tserver/tablet_copy_client.cc@933 PS1, Line 933: consumeWithBorrowAndWait I have no idea what this method does. It would really help if you could share some details on that. Also adding the same information in comments here would prove handy in the long run. -- To view, visit http://gerrit.cloudera.org:8080/19479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f4834bfb0718a2b6b1d946975287a11f6be1fe3 Gerrit-Change-Number: 19479 Gerrit-PatchSet: 1 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 07 Feb 2023 12:46:03 +0000 Gerrit-HasComments: Yes
