You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2020/01/14 21:01:52 UTC
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Thomas Tauber-Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15032
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
The timeout handling code in Connection::HandleOutboundCallTimeout
assumes that when a timeout is triggered the OutboundCall must still
be valid. In the case of an asynchronous rpc that was cancelled before
the timeout was hit, this will not be the case.
The fix is to check if the OutboundCall is still valid and return
without processing the timeout if it is not.
Testing:
- Tested in Impala in the context of implementing IMPALA-8712.
Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
---
M src/kudu/rpc/connection.cc
1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/15032/1
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
The timeout handling code in Connection::HandleOutboundCallTimeout
assumes that when a timeout is triggered the OutboundCall must still
be valid. In the case of an asynchronous rpc that was cancelled before
the timeout was hit, this will not be the case.
The fix is to check if the OutboundCall is still valid and return
without processing the timeout if it is not.
Testing:
- Added timeout tests to TestCancellation to inject timeouts at
OutboundCall states from READY to SENT.
- Tested in Impala in the context of implementing IMPALA-8712.
Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Reviewed-on: http://gerrit.cloudera.org:8080/15032
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
3 files changed, 22 insertions(+), 9 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc@380
PS1, Line 380: The RPC may have been cancelled before the timeout was hit
Good find!
Is it possible to add a repro test scenario for this? How do we know there aren't any other CHECKS/DCHECK() in the control path when this happens?
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 14 Jan 2020 21:33:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:
http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h@536
PS2, Line 536: static void DoTestExpectTimeout(const Proxy& p,
> warning: method 'DoTestExpectTimeout' can be made static [readability-conve
Done
http://gerrit.cloudera.org:8080/#/c/15032/2/src/kudu/rpc/rpc-test-base.h@558
PS2, Line 558: int elapsed_millis = static_cast<int>(sw.elapsed().wall_millis());
> warning: narrowing conversion from 'double' to 'int' [bugprone-narrowing-co
Done
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Jan 2020 23:15:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc@380
PS1, Line 380: The RPC may have been cancelled before the timeout was hit
> Done
Thank you!
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 17 Jan 2020 01:27:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/15032/1/src/kudu/rpc/connection.cc@380
PS1, Line 380: The RPC may have been cancelled before the timeout was hit
> Good find!
Done
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Jan 2020 22:56:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15032
to look at the new patch set (#3).
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
The timeout handling code in Connection::HandleOutboundCallTimeout
assumes that when a timeout is triggered the OutboundCall must still
be valid. In the case of an asynchronous rpc that was cancelled before
the timeout was hit, this will not be the case.
The fix is to check if the OutboundCall is still valid and return
without processing the timeout if it is not.
Testing:
- Added timeout tests to TestCancellation to inject timeouts at
OutboundCall states from READY to SENT.
- Tested in Impala in the context of implementing IMPALA-8712.
Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
3 files changed, 22 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/15032/3
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15032
to look at the new patch set (#2).
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
The timeout handling code in Connection::HandleOutboundCallTimeout
assumes that when a timeout is triggered the OutboundCall must still
be valid. In the case of an asynchronous rpc that was cancelled before
the timeout was hit, this will not be the case.
The fix is to check if the OutboundCall is still valid and return
without processing the timeout if it is not.
Testing:
- Added timeout tests to TestCancellation to inject timeouts at
OutboundCall states from READY to SENT.
- Tested in Impala in the context of implementing IMPALA-8712.
Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
3 files changed, 18 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/15032/2
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15032 )
Change subject: KUDU-3042: Fix invalid DCHECK when rpc is cancelled then times out
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15032
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b97ccc4787809fca8fbc991bb1151940477efca
Gerrit-Change-Number: 15032
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 17 Jan 2020 01:25:20 +0000
Gerrit-HasComments: No