You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/11/30 23:26:25 UTC
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5292
to review the following change.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
KUDU-1773: remove overly strict DCHECKs
A tablet's replicas may change over the course of a write. For example,
between two attempts at the same write, a replica could be evicted from
the replication group and replaced by another.
A pair of DCHECKs in the metacache didn't allow that, so they've been
removed. Furthermore, their respective RemoteTablet functions now don't
return anything, to reflect the fact that success or failure in finding the
replica in question should be immaterial to all callers.
Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
---
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
2 files changed, 2 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5292/1
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
Patch Set 1:
Can you think of a new stress test we could add that would trigger this? It's a shame that we found this issue via the Impala end-to-end stress tests and don't have our own coverage of this race scenario.
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
Patch Set 1:
> Can you think of a new stress test we could add that would trigger
> this? It's a shame that we found this issue via the Impala
> end-to-end stress tests and don't have our own coverage of this
> race scenario.
I'm tempted to punt on this.
It would be useful, but after discussion with MJ it seems like the Impala stress test that uncovered this issue is...far more stressful than any of ours. It uses 8 nodes with an average of 300-600 replicas per node, issues many queries to multiple tables concurrently, and deals with tables up to hundreds of millions of rows in size. On top of that, the test is already dealing with some unexplained timeouts pausing reactor threads for dozens of seconds at a time; if those timeouts are happening client-side, it could certainly explain the interleaving necessary to tease out this race.
To be clear, it's not impossible to write a stress test for this race. The client workload would need to be performing writes with many retries, and be doing "cold" tablet lookups often. On top of that, the servers would need to be cycling through replicas quickly, evicting and replacing them as fast as possible. I think it'd be a fair amount of work to implement that, for relatively little gain (these crashes were debug-only, after all).
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
KUDU-1773: remove overly strict DCHECKs
A tablet's replicas may change over the course of a write. For example,
between two attempts at the same write, a replica could be evicted from
the replication group and replaced by another.
A pair of DCHECKs in the metacache didn't allow that, so they've been
removed. Furthermore, their respective RemoteTablet functions now don't
return anything, to reflect the fact that success or failure in finding the
replica in question should be immaterial to all callers.
Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Reviewed-on: http://gerrit.cloudera.org:8080/5292
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
2 files changed, 2 insertions(+), 15 deletions(-)
Approvals:
Mike Percy: Looks good to me, but someone else must approve
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1773: remove overly strict DCHECKs
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-1773: remove overly strict DCHECKs
......................................................................
Patch Set 1: Code-Review+1
Looks safe to me. I'll leave it to Todd for final review.
--
To view, visit http://gerrit.cloudera.org:8080/5292
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I01c1dde99cce1f43e6a9864c1ff6f7aaad448a77
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No