Mike Percy has posted comments on this change. Change subject: KUDU-1933. consensus: Avoid and repair integer overflow in log index ......................................................................
Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/6376/9/src/kudu/integration-tests/ts_recovery-itest.cc File src/kudu/integration-tests/ts_recovery-itest.cc: PS9, Line 335: INT32_MIN > I forget where these come from, but should we use std::limits, for consiste This is included in <cstdint> as of C++11 however in Kudu we include <stdint.h> both for C++03 compat and so that we don't have to refer to types such as int64_t as std::int64_t. So this actually comes from <stdint.h> which is included anywhere we use types such as int64_t (i.e. everywhere) PS9, Line 389: NO_FATALS(); > isn't the usual practice to wrap AssertEventually in NO_FATALS? It fails to compile under GCC5. Still need to investigate why. See https://gerrit.cloudera.org/6354 PS9, Line 391: Now, > remove Why? PS9, Line 418: NO_FATALS(); > same See above http://gerrit.cloudera.org:8080/#/c/6376/9/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: PS9, Line 800: CHECK_LT(overflow, FLAGS_group_commit_queue_size_bytes) << OpIdToString(*opid); > this is a bit finnicky. The gc queue size could in theory be changed betwee 1. This is a size in bytes, not entries, and the default is 4M. So I believe this is a very comfortable upper bound for catching this issue. You are right that the group commit queue size can be modified but I view that as a good thing because worst case scenario it could be tuned up to accommodate this. However it seems very unlikely to me that a user will decide to do that right after seeing this crash. This check is only triggered when we detect a negative log index value. 2. There is no way multiple batches can go in because there is only one AppendThread and it crashes when updating the indexes right after it appends. -- To view, visit http://gerrit.cloudera.org:8080/6376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I284edbde51dc50fb2f98acc83cdcc3891d37863f Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
