You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2023/08/02 06:00:06 UTC
[kudu-CR] WIP [tablet] don't start operations timed out in prepare queue
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20300
Change subject: WIP [tablet] don't start operations timed out in prepare queue
......................................................................
WIP [tablet] don't start operations timed out in prepare queue
WIP:
* file an upstream JIRA to track this improvement
* write proper commit description
* add the metric for the number of requests timed out in the queue
Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
---
M src/kudu/tablet/ops/op_driver.cc
M src/kudu/tablet/ops/op_driver.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
9 files changed, 165 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/20300/1
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc@1681
PS2, Line 1681: deadline
> I though about that before opting out for the current approach implemented
From the other side, maybe the deadline should be a member of WriteOp itself? I'm not quite sure, but it seems I'm missing some bigger picture here to properly attribute the properties of various objects. At some point, the op's driver seemed as a good place to keep the deadline for the operation (if set).
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 03 Aug 2023 01:33:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
KUDU-3500 don't start operations timed out in prepare queue
While troubleshooting a performance issue where the prepare queue for
a tablet was very long, I noticed that tablet servers start write
operations that correspond to RPCs that have already timed out. Most
likely, the client that sent the RPC had already detected the timeout
and expected that the write would have failed already.
As a simple optimization, this patch updates the logic of the OpDriver
class to respond with TimedOut error status right away when a write
operation that has already timed out while waiting in the prepare queue
is dispatched to the prepare thread. That helps with clearing the queue
and processing not-yet-timed-out requests from the queue faster,
increasing the overall robustness of a tablet server when the load
is high and the node's CPU and disk IO bandwidth are saturated.
A new tablet metric 'ops_timed_out_in_prepare_queue' is introduced to
track the number of WriteRequestPB RPCs timed out in the tablet's prepare
queue and responded with TimedOut error status even before starting
the PREPARE phase for the corresponding operation.
This patch also adds a new test to cover the new functionality.
Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Reviewed-on: http://gerrit.cloudera.org:8080/20300
Tested-by: Kudu Jenkins
Reviewed-by: Abhishek Chennaka <ac...@cloudera.com>
---
M src/kudu/tablet/ops/op_driver.cc
M src/kudu/tablet/ops/op_driver.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 170 insertions(+), 21 deletions(-)
Approvals:
Kudu Jenkins: Verified
Abhishek Chennaka: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc@1681
PS2, Line 1681: deadline
> From the other side, maybe the deadline should be a member of WriteOp itsel
I took another look into the code. The OpDriver takes care of all types of the operations, so there is no need to add the timeout in OpState to have this for any other type of operation.
So, as of PS3 the deadline field seems to be at the right place, IIUC.
Let me know if I'm missing something.
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Sat, 05 Aug 2023 06:08:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tablet/ops/write_op.cc@76
PS2, Line 76: ;
nit: Maybe use .(fullstop) to be consistent with tablet_inject_latency_on_apply_write_op_ms above.
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc@1681
PS2, Line 1681: deadline
What are your thoughts on storing this deadline in a member variable of kudu::tablet::OpState? In that way if we want to extend this to other type of ops in future we would be able to.
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Wed, 02 Aug 2023 21:10:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20300
to look at the new patch set (#3).
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
KUDU-3500 don't start operations timed out in prepare queue
While troubleshooting a performance issue where the prepare queue for
a tablet was very long, I noticed that tablet servers start write
operations that correspond to RPCs that have already timed out. Most
likely, the client that sent the RPC had already detected the timeout
and expected that the write would have failed already.
As a simple optimization, this patch updates the logic of the OpDriver
class to respond with TimedOut error status right away when a write
operation that has already timed out while waiting in the prepare queue
is dispatched to the prepare thread. That helps with clearing the queue
and processing not-yet-timed-out requests from the queue faster,
increasing the overall robustness of a tablet server when the load
is high and the node's CPU and disk IO bandwidth are saturated.
A new tablet metric 'ops_timed_out_in_prepare_queue' is introduced to
track the number of WriteRequestPB RPCs timed out in the tablet's prepare
queue and responded with TimedOut error status even before starting
the PREPARE phase for the corresponding operation.
This patch also adds a new test to cover the new functionality.
Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
---
M src/kudu/tablet/ops/op_driver.cc
M src/kudu/tablet/ops/op_driver.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 170 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/20300/3
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tablet/ops/write_op.cc@76
PS2, Line 76: ;
> nit: Maybe use .(fullstop) to be consistent with tablet_inject_latency_on_a
Done
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc@1681
PS2, Line 1681: deadline
> What are your thoughts on storing this deadline in a member variable of kud
I though about that before opting out for the current approach implemented in the patch and I didn't see the how the deadline fits OpsState with the existing types of operations. We have only WriteOpState where it makes sense, and the rest are AlterSchemaOpState, ParticipantOpState where the timeout doesn't quite fit. By my understanding, even in the case of replicated write operation the timeout isn't exactly fit to be a field/member of the WriteOpState.
Also, I don't believe in the provisions of something for hypothetical future extensions: usually, those "extensions" never happen, or when they happen, you realize the majority of the prep work should be re-done. That's because it's much easier to write the better code once you know exactly what's needed.
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 03 Aug 2023 01:18:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20300 )
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/20300/2/src/kudu/tserver/tablet_service.cc@1681
PS2, Line 1681: deadline
> I took another look into the code. The OpDriver takes care of all types of
Agree. I think this patch looks good to me.
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Mon, 07 Aug 2023 18:06:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3500 don't start operations timed out in prepare queue
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20300
to look at the new patch set (#2).
Change subject: KUDU-3500 don't start operations timed out in prepare queue
......................................................................
KUDU-3500 don't start operations timed out in prepare queue
While troubleshooting a performance issue where the prepare queue for
a tablet was very long, I noticed that tablet servers start write
operations that correspond to RPCs that have already timed out. Most
likely, the client that sent the RPC had already detected the timeout
and expected that the write would have failed already.
As a simple optimization, this patch updates the logic of the OpDriver
class to respond with TimedOut error status right away when a write
operation that has already timed out while waiting in the prepare queue
is dispatched to the prepare thread. That helps with clearing the queue
and processing not-yet-timed-out requests from the queue faster,
increasing the overall robustness of a tablet server when the load
is high and the node's CPU and disk IO bandwidth are saturated.
A new tablet metric 'ops_timed_out_in_prepare_queue' is introduced to
track the number of WriteRequestPB RPCs timed out in the tablet's prepare
queue and responded with TimedOut error status even before starting
the PREPARE phase for the corresponding operation.
This patch also adds a new test to cover the new functionality.
Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
---
M src/kudu/tablet/ops/op_driver.cc
M src/kudu/tablet/ops/op_driver.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 170 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/20300/2
--
To view, visit http://gerrit.cloudera.org:8080/20300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I202ce6b5e425439b50c0751d7f7406e69b8e751a
Gerrit-Change-Number: 20300
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)