[kudu-CR] Add snapshot scans to fuzz-itest
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4996 to look at the new patch set (#11). Change subject: Add snapshot scans to fuzz-itest .. Add snapshot scans to fuzz-itest This adds a new operation to fuzz-itest: snapshot scans at a timestamp. When generating random operations, scans at a timestamp are now in the mix. The generator records how timestamps will progress with writes and is careful to make sure that scans happen in the past. After the test case is generated, but before it is run, the timestamps for the scans are deduplicated and sorted to that we save the state immediately before those (and only those) timestamps. This would fail very often before the REINSERT patches but passes with it. Ran 500 loops of fuzz-itest in dist-test in ASAN with: - KUDU_ALLOW_SLOW_TESTS=1 - --stress_cpu_threads=4 All tests passed. Result: http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319 Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e --- M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/key_value_test_schema.h M src/kudu/tserver/tablet_service.cc 3 files changed, 200 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/11 -- To view, visit http://gerrit.cloudera.org:8080/4996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Reduce the number of batches in FuzzTest::TestFuzzHugeBatches
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/5223 Change subject: Reduce the number of batches in FuzzTest::TestFuzzHugeBatches .. Reduce the number of batches in FuzzTest::TestFuzzHugeBatches While running fuzz-itest in dist-test with slow mode, asan, it's often flaky due to timeouts. This test is the main culprit as it often takes 13 minutes to complete. This reduces the number of operations in this test and 'update_multiplier' to half, making it non-flaky. Change-Id: Ib73220ed342d2417ad65bf1ae705499cab7a9b10 --- M src/kudu/integration-tests/fuzz-itest.cc 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/5223/1 -- To view, visit http://gerrit.cloudera.org:8080/5223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib73220ed342d2417ad65bf1ae705499cab7a9b10 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4995 to look at the new patch set (#17). Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. KUDU-237 (part 2) - Add support for REINSERT in delta files This patch goes the final mile in adding support for REINSERTs in delta files. It does various things to achieve this: - Transforms REDO DELETE/REINSERT mutation pairs into UNDO REINSERT/DELETE mutation pairs, leaving at the most a REDO delete. - Merges ghost rows on compaction: When duplicated rows are found a new algorithm finds out which one is the most recent and adds the other one as a 'previous_ghost'. This can happen for an arbitrary number of ghost rows. Noteworthy is that setting previous versions requires a copy. The two rows are in different RowBlocks (for row data) and Arenas (for mutations) and it is not guaranteed that the previous ghost will suvive by the time the row that points to it is processed. - Adds new test to tablet-test and changes a test in compaction-test proving that this works. - Adds a new test to compaction-test that creates several layers of overlapping rowsets where each layer has one rowset less than the previous one. This creates a mix of duplicated (up to 10 different ghosts) and unique rows. The test then compacts the rowsets a few at a time and makes sure the histories are correct. Follow up patches will add new itests that test this functionality even more broadly. Ran 500 loops of mt-tablet-test and compaction-test in dist-test with "KUDU_ALLOW_SLOW_TESTS=1" and "--stress_cpu_threads=4". All tests passed. Results: compaction-test: http://dist-test.cloudera.org//job?job_id=david.alves.1480028902.31041 mt-tablet-test: http://dist-test.cloudera.org//job?job_id=david.alves.1480028908.31125 Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 --- M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/tablet-test.cc 8 files changed, 663 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/17 -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..
Patch Set 16:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/4995/16/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 266: DCHECK(right.redo_head != nullptr);
> nit: can remove this or I guess should be DCHECK(right_last != nullptr)
Done
Line 272: DCHECK(left.redo_head != nullptr);
> nit: could remove or I guess should be DCHECK(left_last != nullptr)
Done
PS16, Line 463: want
> want to make
Done
Line 689: void MergeUndoHistories(Mutation* left, Mutation* right, Mutation**
head) {
> How about return the head instead of having to pass a Mutation**
Done
Line 1012: << ERROR_LOG_CONTEXT;
> nit: looks like an extra space
Done
http://gerrit.cloudera.org:8080/#/c/4995/16/src/kudu/tablet/deltafile.cc
File src/kudu/tablet/deltafile.cc:
PS16, Line 749: }
> nit: inconsistent missing space before brace
Done
Line 753: LivenessVisitor visitor = { this, sel_vec};
> here too
Done
--
To view, visit http://gerrit.cloudera.org:8080/4995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] [c++ client] timestamp propagation via scan tokens
David Ribeiro Alves has posted comments on this change. Change subject: [c++ client] timestamp propagation via scan tokens .. Patch Set 1: I meant making sure that we're not breaking anything with this change stand-alone. We usually get the scan tokens from the java client and then use then in the c++ client. would that still work with this change? -- To view, visit http://gerrit.cloudera.org:8080/5220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [c++ client] timestamp propagation via scan tokens
David Ribeiro Alves has posted comments on this change. Change subject: [c++ client] timestamp propagation via scan tokens .. Patch Set 1: did you test that these tokens are cross-client compatible? -- To view, visit http://gerrit.cloudera.org:8080/5220 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c76c20b62cb91695c69f7dc4b98f4dc98bf02cc Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [java] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has posted comments on this change. Change subject: [java] Reuse snapshot scan timestamp across tablets .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/5188/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: PS6, Line 650: Server-assigned timestamp for the scan operation. It's used when : * the scan operates in READ_AT_SNAPSHOT mode and the timestamp is not : * specified explicitly. The field is set with the snapshot timestamp sent : * in the response from the very first tablet server contacted while : * fetching data from corresponding tablets. If the tablet server does not : * send the snapshot timestamp in its response, this field is assigned : * a special value AsyncKuduClient.NO_TIMESTAMP. > If so, then it contradicts with the comment for $REPO_ROOT/src/kudu/tserver maybe I didn't explain myself correctly. It was not about adding a new test. It was about adding a checkState() or whatever for the following: If the server sent a timestamp on the response (i.e. if the 'timestamp' field on the response is set or is != from NO_TIMESTAMP or whatever the java client calls it) then: If a timestamp was set on the scanner before, they should match so we should add a check to make sure they do. If a timestamp was not set on the scanner before, set it. also nit: could you rename serverScanTimestamp to responseScanTimestamp? -- To view, visit http://gerrit.cloudera.org:8080/5188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7207672f7b0cf1307bfa861bda3291b278618016 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has posted comments on this change. Change subject: [java] Reuse snapshot scan timestamp across tablets .. Patch Set 6: -Code-Review (1 comment) oops, found a problem http://gerrit.cloudera.org:8080/#/c/5188/6/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: PS6, Line 650: Server-assigned timestamp for the scan operation. It's used when : * the scan operates in READ_AT_SNAPSHOT mode and the timestamp is not : * specified explicitly. The field is set with the snapshot timestamp sent : * in the response from the very first tablet server contacted while : * fetching data from corresponding tablets. If the tablet server does not : * send the snapshot timestamp in its response, this field is assigned : * a special value AsyncKuduClient.NO_TIMESTAMP. isn't it that the server always returns a scan timestamp, even if the user set it. What we should do instead is to make sure that we have a check that they match if the user set it and that we set it if the user hasn't -- To view, visit http://gerrit.cloudera.org:8080/5188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7207672f7b0cf1307bfa861bda3291b278618016 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [java] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has posted comments on this change. Change subject: [java] Reuse snapshot scan timestamp across tablets .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7207672f7b0cf1307bfa861bda3291b278618016 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4995 to look at the new patch set (#16). Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. KUDU-237 (part 2) - Add support for REINSERT in delta files This patch goes the final mile in adding support for REINSERTs in delta files. It does various things to achieve this: - Transforms REDO DELETE/REINSERT mutation pairs into UNDO REINSERT/DELETE mutation pairs, leaving at the most a REDO delete. - Merges ghost rows on compaction: When duplicated rows are found a new algorithm finds out which one is the most recent and adds the other one as a 'previous_ghost'. This can happen for an arbitrary number of ghost rows. Noteworthy is that setting previous versions requires a copy. The two rows are in different RowBlocks (for row data) and Arenas (for mutations) and it is not guaranteed that the previous ghost will suvive by the time the row that points to it is processed. - Adds new test to tablet-test and changes a test in compaction-test proving that this works. - Adds a new test to compaction-test that creates several layers of overlapping rowsets where each layer has one rowset less than the previous one. This creates a mix of duplicated (up to 10 different ghosts) and unique rows. The test then compacts the rowsets a few at a time and makes sure the histories are correct. Follow up patches will add new itests that test this functionality even more broadly. Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 --- M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/tablet-test.cc 8 files changed, 661 insertions(+), 159 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/16 -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4995 to look at the new patch set (#15). Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. KUDU-237 (part 2) - Add support for REINSERT in delta files This patch goes the final mile in adding support for REINSERTs in delta files. It does various things to achieve this: - Transforms REDO DELETE/REINSERT mutation pairs into UNDO REINSERT/DELETE mutation pairs, leaving at the most a REDO delete. - Merges ghost rows on compaction: When duplicated rows are found a new algorithm finds out which one is the most recent and adds the other one as a 'previous_ghost'. This can happen for an arbitrary number of ghost rows. Noteworthy is that setting previous versions requires a copy. The two rows are in different RowBlocks (for row data) and Arenas (for mutations) and it is not guaranteed that the previous ghost will suvive by the time the row that points to it is processed. - Adds new test to tablet-test and changes a test in compaction-test proving that this works. - Adds a new test to compaction-test that creates several layers of overlapping rowsets where each layer has one rowset less than the previous one. This creates a mix of duplicated (up to 10 different ghosts) and unique rows. The test then compacts the rowsets a few at a time and makes sure the histories are correct. Follow up patches will add new itests that test this functionality even more broadly. Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 --- M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/tablet-test.cc 8 files changed, 662 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/15 -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. Patch Set 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/4995/14/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: PS14, Line 261: DCHECK(right.redo_head != nullptr); : const Mutation* right_last = left.redo_head; : AdvanceToLastInList(&right_last); > no need to have the advance code twice with two different vars Done PS14, Line 671: live > the most recent version might not be 'live' either. Done PS14, Line 688: se > typo Done PS14, Line 1021: TODO(dralves) Make Reinserts set defaults on the dest row. > point to the jira number Done PS14, Line 1093: versions > remove reference to "versions" here and elsewhere Done -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. Patch Set 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/4995/14/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: PS14, Line 261: DCHECK(right.redo_head != nullptr); : const Mutation* right_last = left.redo_head; : AdvanceToLastInList(&right_last); no need to have the advance code twice with two different vars PS14, Line 671: live the most recent version might not be 'live' either. PS14, Line 688: se typo PS14, Line 1021: TODO(dralves) Make Reinserts set defaults on the dest row. point to the jira number PS14, Line 1093: versions remove reference to "versions" here and elsewhere -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4995 to look at the new patch set (#14). Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. KUDU-237 (part 2) - Add support for REINSERT in delta files This patch goes the final mile in adding support for REINSERTs in delta files. It does various things to achieve this: - Transforms REDO DELETE/REINSERT mutation pairs into UNDO REINSERT/DELETE mutation pairs, leaving at the most a REDO delete. - Merges ghost rows on compaction: When duplicated rows are found a new algorithm finds out which one is the most recent and adds the other one as a 'previous_ghost'. This can happen for an arbitrary number of ghost rows. Noteworthy is that setting previous versions requires a copy. The two rows are in different RowBlocks (for row data) and Arenas (for mutations) and it is not guaranteed that the previous ghost will suvive by the time the row that points to it is processed. - Adds new test to tablet-test and changes a test in compaction-test proving that this works. - Adds a new test to compaction-test that creates several layers of overlapping rowsets where each layer has one rowset less than the previous one. This creates a mix of duplicated (up to 10 different ghosts) and unique rows. The test then compacts the rowsets a few at a time and makes sure the histories are correct. Follow up patches will add new itests that test this functionality even more broadly. Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 --- M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/tablet-test.cc 8 files changed, 661 insertions(+), 160 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/14 -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Make DebugDumpCompactionInput use CompactionInputRowToString
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/5215 Change subject: Make DebugDumpCompactionInput use CompactionInputRowToString .. Make DebugDumpCompactionInput use CompactionInputRowToString Recent patches changed and unified the way we print rows but this method was left out as multiple tests were depending on the old format. This patch takes this the rest of the way and changes the tests so that they use the new format. Change-Id: I13a83ecdb8f801c7952d6e130db77726f3b14a83 --- M src/kudu/integration-tests/alter_table-test.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/tablet_history_gc-test.cc 4 files changed, 63 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5215/1 -- To view, visit http://gerrit.cloudera.org:8080/5215 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I13a83ecdb8f801c7952d6e130db77726f3b14a83 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. Patch Set 13: (29 comments) http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/common/row_changelist.h File src/kudu/common/row_changelist.h: Line 349 > hmm, why remove these assertions? they've been quite useful in catching bug oh, I guess this is leftover from the version of the patch that didn't have ghosts in multiple DRSs, put them back. PS12, Line 309: > I guess this function is no longer meant to be update-specific, given you r Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: Line 528: "Undos: [@1(DELETE)] " > I think it's worth extending this test or adding a new one which does somet I added a new test that covers this stuff more thoroughly. http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 90: row_block_.reset(new RowBlock(iter_->schema(), num_in_block, &arena_)); > hm, was this a bug? I guess not really since we don't do flushing compactions (where we would need the MRS's arena in MergeCompactionInput::PrepareBlock()) but I figured it couldn't hurt, right? seems silly that we're putting the mutations in the arena but not the copy of the row. Line 255: // Compares two duplicate rows before compaction (and before the REDO->UNDO transformation). > At first before reading the code, I expected there was a precondition here yeah this happen before the transformation and serves the sole purpose of comparing two rows with the same key from two row sets to set 'previous_ghost' appropriately. One of the rows might have a log redo history. Made this more explicit. PS12, Line 256: -1 if 'left' is less recent than 'r > this phrasing is a bit odd. Is it equivalent to say: Returns -1 if 'left' i Done Line 261: DCHECK(right.redo_head != nullptr); > for the purposes of this DCHECK, seems like it's worth actually doing Advan Done PS12, Line 284: return ret; : } : > can you give the specific scenario here? iirc it's: Done PS12, Line 295: > its Done Line 299: // Update row 'a' @ 10 > nit: maybe more intuitive to write this as Done PS12, Line 453: :OK(); > this sounds like you're talking about a pre-existing state, but really you Done Line 589: return false; > is this recursion inherently depth-limited by the number of input rowsets? yeah, there can be at most n-1 ghosts per row where n is the number of rowsets in the compaction input Line 667: Arena* prepared_block_arena_; > doc Done Line 679: }; > doing this recursively seems dangerous, since it's bounded only by the numb it can, you're right, I was lazy. Done PS12, Line 698: > typo Done PS12, Line 708: Bi > typo: is Done Line 969: > nit: indentation seems off on this comment block. Done Line 988: > yea, this seems like a potentially serious issue. Mind filing a critical JI created KUDU-1760 Line 991: } > I know you didn't write this but is it right not to use an arena here? I guess the point here is that there is no advantage in copying the row to the arena since dst_row comes from a row_block on the stack and it's lifetime is already controlled. http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: PS12, Line 167: row is found in mu > or "when a row appears in multiple RowSets" Done PS12, Line 167: row is found in mu > I think the term 'version' here is a bit confusing, because even without th Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: Line 687: // Visitor which establishes the liveness of a row by applying deletes and reinserts. > +1 for LivenessVisitor Done Line 687: // Visitor which establishes the liveness of a row by applying deletes and reinserts. > we should rename this visitor to LivenessVisitor or DeleteReinsertVisitor o Done http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/tablet-test.cc File src/kudu/tablet/tablet-test.cc: PS12, Line 228: row > typo Done PS12, Line 229: ful > s/all/full/ Done Line 237: // Insert one row. > punctuation here and below in the comments Done Line 238: ASSERT_OK(this->InsertTestRow(&writer, 1, 0)); > ASSERT_OK() ? Done Line 239: > No need to check last_op_result(), InsertTestRow() already returns non-OK v Done PS12, Line 255: delet > typo: fourth Done -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerri
[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4995 to look at the new patch set (#13). Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files .. KUDU-237 (part 2) - Add support for REINSERT in delta files This patch goes the final mile in adding support for REINSERTs in delta files. It does various things to achieve this: - Transforms REDO DELETE/REINSERT mutation pairs into UNDO REINSERT/DELETE mutation pairs, leaving at the most a REDO delete. - Merges ghost rows on compaction: When duplicated rows are found a new algorithm finds out which one is the most recent and adds the other one as a 'previous_ghost'. This can happen for an arbitrary number of ghost rows. Noteworthy is that setting previous versions requires a copy. The two rows are in different RowBlocks (for row data) and Arenas (for mutations) and it is not guaranteed that the previous ghost will suvive by the time the row that points to it is processed. - Adds new test to tablet-test and changes a test in compaction-test proving that this works. - Adds a new test to compaction-test that creates several layers of overlapping rowsets where each layer has one rowset less than the previous one. This creates a mix of duplicated (up to 10 different ghosts) and unique rows. The test then compacts the rowsets a few at a time and makes sure the histories are correct. Follow up patches will add new itests that test this functionality even more broadly. Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 --- M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/tablet-test.cc 8 files changed, 661 insertions(+), 163 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/13 -- To view, visit http://gerrit.cloudera.org:8080/4995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [c++] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has submitted this change and it was merged. Change subject: [c++] Reuse snapshot scan timestamp across tablets .. [c++] Reuse snapshot scan timestamp across tablets KUDU-1189 On reads at a snapshot that touch multiple tablets, without the user setting a timestamp, use the timestamp from the first server for following scans. For a READ_AT_SNAPSHOT scan operation with no snapshot timestamp specified, store the snapshot timestamp returned from the first tablet server into the scan configuration object. Then reuse it when continuing the scan on other tablet servers operations performed at other tablet servers. Added corresponding unit test as well. Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Reviewed-on: http://gerrit.cloudera.org:8080/5143 Reviewed-by: David Ribeiro Alves Tested-by: Kudu Jenkins --- M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/integration-tests/consistency-itest.cc 4 files changed, 79 insertions(+), 7 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
David Ribeiro Alves has submitted this change and it was merged. Change subject: KUDU-1189 integration test for reusing snap timestamp .. KUDU-1189 integration test for reusing snap timestamp Added a test for reusing snapshot timestamp when not set while running scans in READ_AT_SNAPSHOT mode. This is a test for the functionality introduced in the context of KUDU-1189. The test is disabled as it currently fails. A follow up patch will fix the bug and enable the test. Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Reviewed-on: http://gerrit.cloudera.org:8080/5163 Reviewed-by: David Ribeiro Alves Tested-by: Kudu Jenkins --- M src/kudu/client/client.h M src/kudu/integration-tests/consistency-itest.cc 2 files changed, 201 insertions(+), 26 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [java] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has posted comments on this change.
Change subject: [java] Reuse snapshot scan timestamp across tablets
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5188/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
PS5, Line 193: /**
:* Server-assigned timestamp for the scan operation. It's used
when
:* the scan operates in READ_AT_SNAPSHOT mode and the
timestamp is not
:* specified explicitly. Initially, this field is assigned a
special value
:* which means 'unset'. The field is set with the snapshot
timestamp sent
:* in the response from the very first tablet server contacted
while
:* fetching data from corresponding tablets.
:*/
: private long serverScanTimestamp =
AsyncKuduClient.NO_TIMESTAMP;
any specific reason you don't want to reuse "htTimestamp"?
PS5, Line 369: // To be used in tests only.
: protected long getServerScanTimestamp() {
: return this.serverScanTimestamp;
: }
I think this is weird. What is the difference between this getter and the one
above? They're both snapshot timestamps...
http://gerrit.cloudera.org:8080/#/c/5188/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java
File
java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java:
PS5, Line 209: int rowCount = 0;
: while (syncScanner.hasMoreRows()) {
: rowCount += syncScanner.nextRows().getNumRows();
: final long ts = scanner.getServerScanTimestamp();
: assertNotEquals(AsyncKuduClient.NO_TIMESTAMP, ts);
: if (isTimestampSet) {
: assertEquals(tsRef, ts);
: } else {
: isTimestampSet = true;
: tsRef = ts;
: }
: }
If we merge htTimestamp with serverScanTimestamp I think this test could go:
- assert htTimestamp is not set
- assert scanner has more rows
- get more rows
- assert htTimestamp is set
- scan the rest of the rows
- assert htTimestamp is the same
--
To view, visit http://gerrit.cloudera.org:8080/5188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7207672f7b0cf1307bfa861bda3291b278618016
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [c++] Reuse snapshot scan timestamp across tablets
David Ribeiro Alves has posted comments on this change. Change subject: [c++] Reuse snapshot scan timestamp across tablets .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 integration test for reusing snap timestamp .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] [consensus] KUDU-1718: Fix few bugs around replica eviction failures
David Ribeiro Alves has posted comments on this change.
Change subject: [consensus] KUDU-1718: Fix few bugs around replica eviction
failures
..
Patch Set 2:
(4 comments)
can you please address the tidy bot nits? I know most are not your fault but
would be nice to fix them anyway
http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
PS2, Line 235: // Analyze the response from peer.
: PeerResponseStatus ps = AnalyzePeerResponse();
: if (PREDICT_FALSE(ps.response != PeerResponseStatus::OK)) {
: VLOG_WITH_PREFIX_UNLOCKED(1) << "Error response from peer: "
<< ps.status.ToString()
: << ": " <<
response_.ShortDebugString();
: if (ps.response == PeerResponseStatus::ERROR_BUT_ALIVE) {
:
queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid());
: }
: ProcessResponseError(ps.status);
can you change this for a switch case where the error cases get a fall through?
something like:
switch (ps) {
case ERROR_BUT_ALIVE:
queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid());
// Fallthrough intended.
case ERROR:
ProcessResponseError(ps.status);
return;
case OK:
// The queue's handling of the peer response may generate IO (reads against
// the WAL) and SendNextRequest() may do the same thing. So we run the rest
// of the response handling logic on our thread pool and not on the reactor
// thread.
Status s = thread_pool_->SubmitClosure(Bind(&Peer::DoProcessResponse,
Unretained(this)));
if (PREDICT_FALSE(!s.ok())) {
LOG_WITH_PREFIX_UNLOCKED(WARNING) << "Unable to process peer response: " <<
s.ToString()
<< ": " << response_.ShortDebugString();
sem_.Release();
}
http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/consensus/consensus_peers.h
File src/kudu/consensus/consensus_peers.h:
PS2, Line 58: PeerResponseStatus
thanks for adding this.
http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
PS2, Line 2695: // Tests KUDU-1613 and KUDU-1407 fixes when followers aren't
evicted.
please be a little more discriptive
http://gerrit.cloudera.org:8080/#/c/5111/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS2, Line 2589: VLOG(4) << "Error deleting the tablet " << tablet_id() << ": "
: << resp_.DebugString();
should we use a higher log level here or is there enough information in the
statements below?
--
To view, visit http://gerrit.cloudera.org:8080/5111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d3f84fd297a8e4760208a213c1ee393e92499a3
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 integration test for reusing snap timestamp .. Patch Set 4: Does this need to be rebased on top of the patch that unbroke the build? -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1189 scans: reuse snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 scans: reuse snapshot timestamp when not set .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5143/5//COMMIT_MSG Commit Message: PS5, Line 7: scans: reuse snapshot timestamp when not set > We follow that guideline at the authors discretion if you look at the commi or even better: [c++] Reuse snapshot scan timestamp across tablets then you can use [java] for the other patch (which currently has the exact same name) -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 scans: reuse snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 scans: reuse snapshot timestamp when not set .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5143/5//COMMIT_MSG Commit Message: PS5, Line 7: scans: reuse snapshot timestamp when not set > Too long: far more than 50 symbols :) We follow that guideline at the authors discretion if you look at the commit log. Commit message titles are important in the sense that they are the only things that appear on a condensed log view. That being said it was more about rephasing that anything else. How about: Reuse snapshot scan timestamp across tablets -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 integration test for reusing snap timestamp
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 integration test for reusing snap timestamp .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/5163/3/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS3, Line 232: / Advance first partition's tablet server clock nit: this advances the clock of any partition, right? -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [scan] test for reusing snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: [scan] test for reusing snapshot timestamp when not set .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS2, Line 435: shared_ptr client; : ASSERT_OK(cluster_->CreateClient(nullptr, &client)); : // Scan the table at a snapshot: let the servers pick the timestamp. : shared_ptr table; : ASSERT_OK(client->OpenTable(table_name_, &table)); > That would decrease the boilerplate code in this test a bit (one less line ah missed you were using the client below (thought for a sec it was only the scanner). makes sense to leave it then PS2, Line 490: // Insert an additional row into the first tablet. : ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1)); > This is to check that the timestamp is taken from the first tablet server: ah, makes sense. sgtm -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 scans: reuse snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 scans: reuse snapshot timestamp when not set .. Patch Set 1: I'm not sure what kind of test you're planning but in the past, for these types of multi-client changes we've added full-on testing to on client (unit+integration+whatever else) and only unit to the other. So likely no need to go beyond unit here, since you've added a pretty thorough test to the cPP client. JD wdyt? -- To view, visit http://gerrit.cloudera.org:8080/5188 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7207672f7b0cf1307bfa861bda3291b278618016 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [scan] test for reusing snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: [scan] test for reusing snapshot timestamp when not set .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5163/2//COMMIT_MSG Commit Message: PS2, Line 7: [scan] test for reusing snapshot timestamp when not set maybe mention the jira. something like: KUDU- Integration test for timestamp reuse on snapshot scans with no timestamp set. PS2, Line 12: The test would fail because the fix for KUDU-1189 is absent, that's why : it's disabled for a while. The fix is coming in one of the next changes, : which will enable the test. How about: The test is disabled as it currently fails. A follow up patch will fix the bug and enable the test. http://gerrit.cloudera.org:8080/#/c/5163/2/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: PS2, Line 226: UpdateClockForTabletAtKey How about: UpdateClockForTabletHostingKey PS2, Line 297: missing a word? "about"? PS2, Line 435: shared_ptr client; : ASSERT_OK(cluster_->CreateClient(nullptr, &client)); : // Scan the table at a snapshot: let the servers pick the timestamp. : shared_ptr table; : ASSERT_OK(client->OpenTable(table_name_, &table)); nit add a CreateClientAndOpenTable helper? PS2, Line 490: // Insert an additional row into the first tablet. : ASSERT_OK(InsertTestRows(client.get(), table.get(), 1, 1)); what is the point of the additional write? PS2, Line 521: const uint64_t ts_scan = cfg.snapshot_timestamp(); : const uint64_t ts_lo = client->GetLatestObservedTimestamp(); : const MonoDelta t_diff = t_after_scan - t_before_scan; : // An additional check that relies on the assumption that : // the difference between servers' clocks is much bigger compared : // with the scan time. If the assumption does not hold, this check : // does not prove anything, but it does not fail neither. : ASSERT_GE(t_diff.ToMicroseconds(), ts_lo - ts_scan); sorry I missed this before, but isn't this comparing a hybrid time difference with a physical time difference? That doesn't sound right and tbh I don't think this check is adding much to the test. Same goes for the previous block. -- To view, visit http://gerrit.cloudera.org:8080/5163 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7282976580cc15ef330871a838bbf7e46230ceb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 scans: reuse snapshot timestamp when not set
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1189 scans: reuse snapshot timestamp when not set .. Patch Set 5: (2 comments) any way we can add simple unit test for this? http://gerrit.cloudera.org:8080/#/c/5143/5//COMMIT_MSG Commit Message: PS5, Line 7: scans: reuse snapshot timestamp when not set How about: On READ_AT_SNAPSHOT scans, reuse timestamp from first server when not set Line 11: for following scans missing period -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Check that the set raw snapshot timestamp is > 0
David Ribeiro Alves has submitted this change and it was merged. Change subject: Check that the set raw snapshot timestamp is > 0 .. Check that the set raw snapshot timestamp is > 0 Commit 06bb52d changed the default (invalid) timestamp to 0 which was previously a valid raw snapshot timestamp to set. This adds a check to make sure we now return Status::IllegalState in this case. Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 Reviewed-on: http://gerrit.cloudera.org:8080/5155 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/client/client.cc 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5155 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Check that the set raw snapshot timestamp is > 0
David Ribeiro Alves has posted comments on this change. Change subject: Check that the set raw snapshot timestamp is > 0 .. Patch Set 1: not sure, nothing changed in the java client, but having the c++ not allow scans at 0 and allowing them in java seems silly. wdyt? -- To view, visit http://gerrit.cloudera.org:8080/5155 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Don't output unobservable rows from the MemRowset
David Ribeiro Alves has posted comments on this change. Change subject: Don't output unobservable rows from the MemRowset .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG Commit Message: Line 28: for the row. > Yea, I agree it's impossible to establish which one is older, but you can o yeah, I had considered that (both are deleted and really close to the ahm so likely not observable, which is why I didn't come up with a user observable scenario) but honestly I really like the check that makes sure we can always establish which version of a row comes first. Specially as we historically miss some edge cases. So it was between giving that up or doing something super silly (like making sure both rows are exactly in this scenario at which point why would they be in the compaction input anyway) or adding this. I think adding this is unimpactful since the row is unobservable anyway. -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Don't output unobservable rows from the MemRowset
David Ribeiro Alves has posted comments on this change. Change subject: Don't output unobservable rows from the MemRowset .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/4994/9//COMMIT_MSG Commit Message: Line 28: for the row. > still not 100% sure I understand the issue here. Here's what I think the st (unrelated nit) Isn't it more like: DRS1 (original flushed MRS): undos: delete @ 1 base: v1 redos: delete @ 3 MRS2 in memory: undo: delete @ 3 base: v2 redo: delete @ 3 After flushing MRS2, becomes DRS2: undo: delete @ 3 base: v3 redo: delete @ 3 right, my point was not about the scan. it spawned from the reinsert part 2 patch and it was about the fact that the rows get to a state where it's indistinguishable which one is the older version. Say that after two delta compactions with garbage collection we get: DRS1: base: v1 redos: delete @ 3 DRS2: base: v3 redo: delete @ 3 It's impossible to establish which one is oldest -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
David Ribeiro Alves has submitted this change and it was merged. Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Don't do UNDO garbage collection until after the REDO->UNDO transformation Reasoning about what has been garbage collected and not while at the same time generating UNDOs is tricky. Particularly taking REINSERTS into account and multiple row versions merging (part of a follow up patch). This moves UNDO garbage collection to after the transformation and simplifies things a bit, at the cost of some extra work. Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Reviewed-on: http://gerrit.cloudera.org:8080/4993 Reviewed-by: Todd Lipcon Tested-by: David Ribeiro Alves --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc 3 files changed, 72 insertions(+), 96 deletions(-) Approvals: David Ribeiro Alves: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has submitted this change and it was merged. Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param All the other methods in this class, of this genre, take one but this one was taking the raw faststring buffer instead. Though it did have the advantage of not forcing the caller to clear() the buffer everytime it was weirdly out of place and might introduce subtle bugs with new semantics for RowChangeListEncoder. Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Reviewed-on: http://gerrit.cloudera.org:8080/4928 Reviewed-by: Todd Lipcon Tested-by: David Ribeiro Alves --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/tablet/memrowset.cc 3 files changed, 13 insertions(+), 13 deletions(-) Approvals: David Ribeiro Alves: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
David Ribeiro Alves has posted comments on this change. Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Patch Set 8: Verified+1 unrelated flake OpenReadonlyFsITest.TestWriteAndVerify -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has posted comments on this change. Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Patch Set 13: Verified+1 unrelated flake RpcBench.BenchmarkCallsAsync -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
Hello Jean-Daniel Cryans, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4928 to look at the new patch set (#12). Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param All the other methods in this class, of this genre, take one but this one was taking the raw faststring buffer instead. Though it did have the advantage of not forcing the caller to clear() the buffer everytime it was weirdly out of place and might introduce subtle bugs with new semantics for RowChangeListEncoder. Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/tablet/memrowset.cc 3 files changed, 13 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/4928/12 -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has posted comments on this change.
Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take
a RowChangeListEncoder as an out param
..
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4928/10/src/kudu/common/row_changelist.cc
File src/kudu/common/row_changelist.cc:
Line 171:RowChangeListEncoder*
out) {
> probably should DCHECK that 'out' is uninitialized if that's the intention
Done
http://gerrit.cloudera.org:8080/#/c/4928/10/src/kudu/common/row_changelist.h
File src/kudu/common/row_changelist.h:
PS10, Line 359: // The buffer will be cleared before adding the result and
will be empty if the projection
: // ends up producing an empty RowChangeList.
:
> this comment needs to be updated -- probably should mention whether 'out' s
Done
--
To view, visit http://gerrit.cloudera.org:8080/4928
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has posted comments on this change. Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Patch Set 11: -Code-Review oops missed todd's comments. reverting -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has posted comments on this change. Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Patch Set 11: Code-Review+2 again, keeping the +2 -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-798 (part 1) - Unify leader/follower mvcc behavior
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-798 (part 1) - Unify leader/follower mvcc behavior .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/5055/6//COMMIT_MSG Commit Message: PS6, Line 30: This patch series aims at separating the two concepts and fixing : safe time advancement: : a > I like the general direction here. Not sure about this problem. It's still a subset: all "clean" timestamps are "safe", that is we won't advance clean past safe (otherwise we might get in trouble, like you hinted at) it just that we can't only rely on "clean" to advance when waiting for a repeatable read. For instance say that a follower has a clean timestamp of 5, with no writes going on. The leader then updates the safe time to 10. We can't move "clean" to 10 since another leader might come up with operations that come before that (well not until we have leader leases anyway) so what we do is we advance safe time to 10 and keep clean time where it is. >From another angle, until we have leader leases, safe time might wobble back >and forth, we need to make sure that clean time never does that. so we keep >clean time at the time of the last committed txn. When we have leader leases we can actually trust that safe time won't ever go back but having this separation of concerns is still very useful. For instance the time manager (the safe time keeping entity) is knowledgeable about some consensus information like lag and who the leader is, which we can use to better inform clients on what to do when they try snapshot scans on a replica. -- To view, visit http://gerrit.cloudera.org:8080/5055 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3ba7212f9211f585d4bef00e5ccfc24d5eece224 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Check that the set raw snapshot timestamp is > 0
David Ribeiro Alves has posted comments on this change. Change subject: Check that the set raw snapshot timestamp is > 0 .. Patch Set 1: fuzz-itest used to create snapshots at timestamp 0, which previously were silently transformed to no "no timestamp" snapshot scans. This check avoids the silent transformation. looking back we probably shouldn't have changed the int64's to uint64's. On the server the "no timestamp" value is not 0 and we never do a max(no_timestamp, some_timestamp) like we do on the client. -- To view, visit http://gerrit.cloudera.org:8080/5155 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Don't output unobservable rows from the MemRowset
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4994 to look at the new patch set (#9). Change subject: Don't output unobservable rows from the MemRowset .. Don't output unobservable rows from the MemRowset In some rare cases we might have a row that is inserted and deleted in the same operation, meaning the insert and delete have the same timestamp. In this rare case we might run into trouble later on with compactions as there is a sequence of operations that that produce two ghost rows which are uncomparable in terms of recency. The sequence is as follows: Insert Row at 1 -> Row goes in the MRS Flush -> Row now lives in RS1 Delete Row at 3 -> Row is now a ghost in RS1 Insert Row at 3 -> Row goes in the MRS again Delete Row at 3 -> Row is deleted from the MRS Flush -> Row is now a ghost in RS2 Major delta compact RS1 -> GC all before 3 Major delta compact RS2 -> GC all before 3 If RS1 and RS2 now get compacted together there is no way to distinguish Ghost 1 from Ghost 2 and to build a correct history for the row. This adds a small test to compaction-test that makes sure that a row that is inserted and deleted in the same transaction doesn't appear in the compaction input. Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc 2 files changed, 177 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/9 -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't output unobservable rows from the MemRowset
David Ribeiro Alves has posted comments on this change. Change subject: Don't output unobservable rows from the MemRowset .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/4994/6//COMMIT_MSG Commit Message: PS6, Line 18: Flush -> Row now lives in RS1 : Delete Row at 3 -> Row is now a ghost in RS1 : Insert Row at 3 -> Row goes in the MRS again : D > not following how this part of the sequence could happen. A single operatio yeah, I was trying to simplify the sequence and apparently simplified it to a point where it's impossible to happen :) fixed this and also illustrated the sequence in the test. -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Don't output unobservable rows from the MemRowset
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4994 to look at the new patch set (#8). Change subject: Don't output unobservable rows from the MemRowset .. Don't output unobservable rows from the MemRowset In some rare cases we might have a row that is inserted and deleted in the same operation, meaning the insert and delete have the same timestamp. In this rare case we might run into trouble later on with compactions as there is a sequence of operations that that produce two ghost rows which are uncomparable in terms of recency. The sequence is as follows: Insert Row at 1 -> Row goes in the MRS Flush -> Row now lives in RS1 Delete Row at 3 -> Row is now a ghost in RS1 Insert Row at 3 -> Row goes in the MRS again Delete Row at 3 -> Row is delete from the MRS Flush -> Row is now a ghost in RS2 Major delta compact RS1 -> GC all before 3 Major delta compact RS2 -> GC all before 3 If RS1 and RS2 now get compacted together there is no way to distinguish Ghost 1 from Ghost 2 and to build a correct history for the row. This adds a small test to compaction-test that makes sure that a row that is inserted and deleted in the same transaction doesn't appear in the compaction input. Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc 2 files changed, 177 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/8 -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4993 to look at the new patch set (#7). Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Don't do UNDO garbage collection until after the REDO->UNDO transformation Reasoning about what has been garbage collected and not while at the same time generating UNDOs is tricky. Particularly taking REINSERTS into account and multiple row versions merging (part of a follow up patch). This moves UNDO garbage collection to after the transformation and simplifies things a bit, at the cost of some extra work. Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc 3 files changed, 72 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/7 -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
David Ribeiro Alves has posted comments on this change. Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: PS6, Line 627: ahm > I think this comment would be more useful if it explained the intent a bit Done PS6, Line 679: : // Convert the redos into undos. Any redos that are eligible for history GC : // are applied and then thrown away (they don't generate undos). > is this comment still relevant? Done Line 693: Mutation* current_undo; > does this need to be defined up here anymore? only seeing it used in one sc yeah, in a follow up I also use in the reinsert block, mind if I keep it here until then? PS6, Line 699: \ > nit: extra space Done Line 724: CHECK(redo_delete == nullptr); > nit: CHECK_EQ The compiler complains about this: error: use of overloaded operator '<<' is ambiguous (with operand types 'std::ostream' (aka 'basic_ostream') and 'const nullptr_t') Is this something we usually do or am I missing something. PS6, Line 727: undo_encoder.SetToDelete(); > this is old code, but why do we need to use undo_encoder here at all, vs ju we don't, but this will change in a follow up patch so I'd rather leave it and fix it there, if thats ok Line 754: // Reset the UNDO head, losing all previous undos. > we've lost the 'num_rows_history_truncated' thing now. I guess you're figur exactly. http://gerrit.cloudera.org:8080/#/c/4993/6/src/kudu/tablet/compaction.h File src/kudu/tablet/compaction.h: PS6, Line 175: Mutation** redo_head > are the redos modified by this method? if so, should clarify in the doc. Ot Done PS6, Line 187: // 'history_gc_opts': Options indicating whether row history should be : // garbage-collected. > no longer an argument Done -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-237 (part 1) - Support proper mutation encoding for reinserts
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-237 (part 1) - Support proper mutation encoding for
reinserts
..
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4791/16/src/kudu/common/row_changelist-test.cc
File src/kudu/common/row_changelist-test.cc:
Line 150: EXPECT_EQ(string("REINSERT col2=world, col3=12345, col4=NULL"),
> I'm surprised not to see col1=hello here. Can you add a comment why it's no
yeah, added the note you suggested.
http://gerrit.cloudera.org:8080/#/c/4791/16/src/kudu/common/row_changelist.h
File src/kudu/common/row_changelist.h:
Line 221: // it to work for REINSERT and UPDATE.
> nit: can you say 'both REINSERT and UPDATE'?
Done
Line 250: const ColumnSchema& col_schema = schema->column_by_id(col_id);
> this is producing another map lookup, I think better to just use schema->co
Done
http://gerrit.cloudera.org:8080/#/c/4791/16/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
Line 634: namespace { // anonymous namespace
> this '// anonymous namespace' comment belongs with the closing brace below
Done
--
To view, visit http://gerrit.cloudera.org:8080/4791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-237 (part 1) - Support proper mutation encoding for reinserts
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4791 to look at the new patch set (#17). Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts .. KUDU-237 (part 1) - Support proper mutation encoding for reinserts Currently reinserts are not properly encoded like the other mutations and instead refer to an in-memory row, meaning we can't flush them as they don't include the indirect data. This patch makes it so that reinserts are properly encoded, like other mutations but does not yet include the step of flushing them to disk, which will be done in a follow-up patch. Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 --- M src/kudu/common/row_changelist-test.cc M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc 6 files changed, 295 insertions(+), 238 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4791/17 -- To view, visit http://gerrit.cloudera.org:8080/4791 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 Gerrit-PatchSet: 17 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Check that the set raw snapshot timestamp is > 0
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/5155 Change subject: Check that the set raw snapshot timestamp is > 0 .. Check that the set raw snapshot timestamp is > 0 Commit 06bb52d changed the default (invalid) timestamp to 0 which was previously a valid raw snapshot timestamp to set. This adds a check to make sure we now return Status::IllegalState in this case. Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 --- M src/kudu/client/client.cc 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5155/1 -- To view, visit http://gerrit.cloudera.org:8080/5155 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ibc5997f226752d59c56104b26e9b58b8640d0298 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves
[kudu-CR] Add snapshot scans to fuzz-itest
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4996 to look at the new patch set (#10). Change subject: Add snapshot scans to fuzz-itest .. Add snapshot scans to fuzz-itest This adds a new operation to fuzz-itest: snapshot scans at timestamp. When generating random operations scans at a timestamp are now in the mix. The generator records how timestamps will progress with writes and is careful to make sure that scans happen in the past. After the test case is generated, but before it is run, the timestamps for the scans are deduplicated and sorted to that we save state at those (and only those) timestamps while running. This would fail very often before the REINSERT patches but passes with it. Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e --- M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/key_value_test_schema.h M src/kudu/tserver/tablet_service.cc 3 files changed, 191 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/10 -- To view, visit http://gerrit.cloudera.org:8080/4996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add Reinserts to tablet history gc-itest
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4997 to look at the new patch set (#10). Change subject: Add Reinserts to tablet_history_gc-itest .. Add Reinserts to tablet_history_gc-itest This adds Reinserts as a new operation to tablet_history_gc-itest. Reinserts happen on previously deleted rows and, like deletes, have a change to happen mid-compaction. This test used to fail frequently before the reinsert patches are in and now passes. WIPish Will likely get a few dist-test runs for this, though this was already flaky so we'll see how troublesome that is.. Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa --- M src/kudu/integration-tests/tablet_history_gc-itest.cc 1 file changed, 90 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4997/10 -- To view, visit http://gerrit.cloudera.org:8080/4997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add snapshot scans to fuzz-itest
David Ribeiro Alves has posted comments on this change. Change subject: Add snapshot scans to fuzz-itest .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/4996/8//COMMIT_MSG Commit Message: Line 23: WIPish: This is just missing some dist-test runs but should > Still the case? nah, I ran a dist-test a while ago. thanks for the reminder. http://gerrit.cloudera.org:8080/#/c/4996/8/src/kudu/integration-tests/fuzz-itest.cc File src/kudu/integration-tests/fuzz-itest.cc: Line 575: LOG(INFO) << "Cur val before: " << cur_val[test_op.val]; > Did you want to keep those LOGs? Done -- To view, visit http://gerrit.cloudera.org:8080/4996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Don't output unobservable rows from the MemRowset
David Ribeiro Alves has posted comments on this change. Change subject: Don't output unobservable rows from the MemRowset .. Patch Set 6: Verified+1 unrelated flake DebugUtilTest.TestSignalStackTrace -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/5056/3/src/kudu/tablet/local_tablet_writer.h File src/kudu/tablet/local_tablet_writer.h: PS3, Line 118: Since this a "local" tablet writer it shouldn't care about "safe" : // time, which is a distributed concept. > per my comment on the other commit, I'm a little nervous that there is some note that this patch series, so far, does not change the behavior of followers, just the behavior of leaders. Leaders used to move safe time on commit but now all replicas behave the same way and safe time is only adjusted on the TransactionManager. Interestingly we were advancing the safe time on the TransactionManager both for leaders and followers before the apply started which meant that even leader transactions would only advance the clean time and not the safe time on commit. So really this patch series doesn't introduce any change in behavior. -- To view, visit http://gerrit.cloudera.org:8080/5056 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Add snapshot scans to fuzz-itest
David Ribeiro Alves has uploaded a new patch set (#9). Change subject: Add snapshot scans to fuzz-itest .. Add snapshot scans to fuzz-itest This adds a new operation to fuzz-itest: snapshot scans at timestamp. When generating random operations scans at a timestamp are now in the mix. The generator records how timestamps will progress with writes and is careful to make sure that scans happen in the past. After the test case is generated, but before it is run, the timestamps for the scans are deduplicated and sorted to that we save state at those (and only those) timestamps while running. This would fail very often before the REINSERT patches but passes with it. WIPish: This is just missing some dist-test runs but should otherwise be ready for review. Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e --- M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/tablet/key_value_test_schema.h M src/kudu/tserver/tablet_service.cc 3 files changed, 194 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/9 -- To view, visit http://gerrit.cloudera.org:8080/4996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't output unobservable rows from the MemRowset
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4994 to look at the new patch set (#6). Change subject: Don't output unobservable rows from the MemRowset .. Don't output unobservable rows from the MemRowset In some rare cases we might have a row that is inserted and deleted in the same operation, meaning the insert and delete have the same timestamp. In this rare case we might run into trouble later on with compactions as there is a sequence of operations that that produce two ghost rows which are uncomparable in terms of recency. The sequence is as follows: Insert Row at 1 Delete Row at 3 Flush - Ghost 1 now lives in RS1 Insert Row at 3 Delete Row at 3 Flush - Ghost 2 now lives in RS2 Major delta compact RS1 - GC all before 3 Major delta compact RS2 - GC all before 3 If RS1 and RS2 now get compacted together there is no way to distinguish Ghost 1 from Ghost 2 and to build a correct history for the row. This adds a small test to compaction-test that makes sure that a row that is inserted and deleted in the same transaction doesn't appear in the compaction input. Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc 2 files changed, 147 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/6 -- To view, visit http://gerrit.cloudera.org:8080/4994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
David Ribeiro Alves has posted comments on this change. Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Patch Set 10: Code-Review+2 just a rebase keeping the +2 -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-237 (part 1) - Support proper mutation encoding for reinserts
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4791 to look at the new patch set (#16). Change subject: KUDU-237 (part 1) - Support proper mutation encoding for reinserts .. KUDU-237 (part 1) - Support proper mutation encoding for reinserts Currently reinserts are not properly encoded like the other mutations and instead refer to an in-memory row, meaning we can't flush them as they don't include the indirect data. This patch makes it so that reinserts are properly encoded, like other mutations but does not yet include the step of flushing them to disk, which will be done in a follow-up patch. Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 --- M src/kudu/common/row_changelist-test.cc M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/tablet/compaction.cc M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/memrowset.cc 6 files changed, 293 insertions(+), 238 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/4791/16 -- To view, visit http://gerrit.cloudera.org:8080/4791 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param
Hello Jean-Daniel Cryans, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4928 to look at the new patch set (#10). Change subject: Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param .. Make RowChangeListDecoder::RemoveColumnIdsFromChangeList() take a RowChangeListEncoder as an out param All the other methods in this class, of this genre, take one but this one was taking the raw faststring buffer instead. Though it did have the advantage of not forcing the caller to clear() the buffer everytime it was weirdly out of place and might introduce subtle bugs with new semantics for RowChangeListEncoder. Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a --- M src/kudu/common/row_changelist.cc M src/kudu/common/row_changelist.h M src/kudu/tablet/memrowset.cc 3 files changed, 11 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/4928/10 -- To view, visit http://gerrit.cloudera.org:8080/4928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6584b22662b58227a80eaebb7d89199d3ea2498a Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4993 to look at the new patch set (#6). Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Don't do UNDO garbage collection until after the REDO->UNDO transformation Reasoning about what has been garbage collected and not while at the same time generating UNDOs is tricky. Particularly taking REINSERTS into account and multiple row versions merging (part of a follow up patch). This moves UNDO garbage collection to after the transformation and simplifies things a bit, at the cost of some extra work. Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e --- M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc 3 files changed, 63 insertions(+), 90 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/4993/6 -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Don't do UNDO garbage collection until after the REDO->UNDO transformation
David Ribeiro Alves has posted comments on this change. Change subject: Don't do UNDO garbage collection until after the REDO->UNDO transformation .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/4993/2/src/kudu/tablet/compaction.cc File src/kudu/tablet/compaction.cc: Line 619: // If the row has a DELETE redo which is older than ahm, is_garbage_collected is set to true. > Shouldn't this be in the header file? I also found this comment confusing, you mean the comment? done. the variable name is the same as it was previously in ApplyMutationsAndGenerateUndos. it means that the row is garbage collected and shouldn't be output in the compaction output. I moved the relevant comment from that method here (to the header of this method, that is). -- To view, visit http://gerrit.cloudera.org:8080/4993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10e4bbb1cac68474756ea1716cfc4c5420cf503e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-237 (part 1) - Support proper mutation encoding for reinserts
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-237 (part 1) - Support proper mutation encoding for
reinserts
..
Patch Set 15:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4791/15/src/kudu/common/row_changelist-test.cc
File src/kudu/common/row_changelist-test.cc:
Line 201: RowChangeListDecoder decoder(RowChangeList(Slice("\x03")));
> I know you didn't do this but instead of Slice("\x03") it would be nicer to
this doesn't actually work. the string in your suggestion ends up with the
incorrect type (i tried).
http://gerrit.cloudera.org:8080/#/c/4791/15/src/kudu/common/row_changelist.h
File src/kudu/common/row_changelist.h:
Line 163: SetType(RowChangeList::kReinsert);
> Does anything in the encoder protect us from having an incomplete but nonem
not really, but rlcs for reinserts are always built from full rows.
Line 169: void SetToReinsert(const RowType& src_row);
> How about EncodeReinsert()
would prefer to avoid externally visible renaming refactorings since that makes
it harder for patches down the line. happy to rename after the series if you
fell strongly about it.
Line 222: void EncodeUpdate(const ColumnSchema& col_schema,
> How about EncodeColumnMutation()
Done
Line 235: void EncodeUpdateRaw(int col_id, bool is_null, Slice new_val);
> How about EncodeColumnMutationRaw()
Done
Line 332: RowChangeListEncoder* out);
> nit: why change the variable name from undo_encoder to 'out'? The previous
because what's in 'out' is the old state of the row and not an undo. for
instance if you do this on a delete you don't get a reinsert encoded back, so
undo is a misnomer (see compaction.cc ApplyMutationsAndGenerateUndos)
--
To view, visit http://gerrit.cloudera.org:8080/4791
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b0a95e3f618bf20eb18ca4f4b03b9e2ce5704e8
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Jean-Daniel Cryans
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1189 if not set, use timestamp from first server
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1189 if not set, use timestamp from first server
..
Patch Set 3:
(33 comments)
http://gerrit.cloudera.org:8080/#/c/5143/3//COMMIT_MSG
Commit Message:
PS3, Line 7: if not set, use timestamp from first server
Maybe rephrase to: Snapshot scans: reuse snapshot timestamp when not set
(i know it's long but at least it's informative)
PS3, Line 9: KUDU-1189 On reads at a snapshot that touch multiple tablets,
without
: the user setting a timestamp, use the timestamp from the first
server
: for following scans
weird wrapping, I guess you were going for an "extended" title. maybe not
necessary anymore?
PS3, Line 15: read
s/read/scan
PS3, Line 15:
nit: extra space
also maybe: _Then_ reuse it when continuing the scan on other tablet servers.
PS3, Line 19: more consistency
what is "more consistency"? :)
I would say that fixes a bug where we wouldn't be scanning at a snapshot at all.
Is this done on the java client?
http://gerrit.cloudera.org:8080/#/c/5143/3/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS3, Line 75: TimestampPropagationTest
I missed this in the previous patch, but can you rename this test to
ConsistencyITest?
1 - All itests have the ITest suffix
2 - This is a good platform for other stuff that is not explicitely related to
timestamp propagation
PS3, Line 164: GetTabletIdForKeyValue
not from this patch, but maybe rephrase to GetTabletIdsForKeyIntervals?
Also maybe add a GetTabletIdForKey that reuses this, takes a single key and
returns a single tablet (asserting on the latter)
Finally if table_name_ is a field maybe not pass it?
PS3, Line 273: TwoBatchesAndReadAtSnapshot
missed this on the previous patch. after the main test class rename can you
rename this to: TestTimestampPropagationFromScans
PS3, Line 352: KUDU-1189
to close this we also need to do it on the java client. is that already done ,
or on your TODO list?
PS3, Line 354: In the context of the same scan
: // operation, that timestamp is then used as the snapshot
timestamp for the rest
: // of the operations to read data from appropriate tablet
servers.
nit: maybe rephrase this to: If the scan spans multiple tablets, the timestamp
picked when scanning the first tablet is then used when scanning following
tablets.
PS3, Line 358: The idea of the test is simple: have a scan spanned across two
tablets
: // where the clocks of the corresponding tablet servers are
skewed.
I think we could do a better job explaining what exactly this test is doing
here. took me a few minutes to understand.
PS3, Line 360: ServerAssignedScanTimestamp
does this test fail 100% without the rest of the patch?
PS3, Line 360: ServerAssignedScanTimestamp
rename to: TestSnapshotScanTimestampReuse
PS3, Line 360: TEST_F(TimestampPropagationTest, ServerAssignedScanTimestamp) {
FWYW I think the approach we did for the previous patch where we're pushing the
test before the fix is better here. It allows anyone to download the test, run
it without changing code and then download the fix and make sure the test
passes.
PS3, Line 361: maximum delta
you mean maximum error? also not really maximum since your dividing by 2 right?
PS3, Line 363: delta
s/delta/offset ? or something
PS3, Line 384: dynamic_cast(peer->clock())
nit: is cases such as this you could use: HybridClock* clock =
DCHECK_NOTNULL(dynamic_cast(peer->clock()));
PS3, Line 398: were
s/were/was
Line 401: // Now, perform the scan at READ_AT_SNAPSHOT where timestamp is not
specified:
where _a_ timestamp
PS3, Line 409: to
remove "to"
PS3, Line 417: size_t
still not totally sure why you're using 'size_t' here.
PS3, Line 422: ,
At this point (choose one: we/the test has) inserted...
PS3, Line 440: ASSERT_LE(0, t_diff.ToMicroseconds());
what is this trying to "sanity check"? also the comparison is slightly weird,
maybe change it to: ASSERT_GE(t_diff.ToMicroseconds(), 0);
PS3, Line 452: Swap the servers in the time scale:
remove
PS3, Line 453: deltas
replace deltas with something else
PS3, Line 464: parition
typo
PS3, Line 465: dynamic_cast(peer->clock());
use DCHECK_NOTNULL
PS3, Line 467: two_deltas
why "two_deltas"?
PS3, Line 469: UpdateClock
Change/Add AdvanceClockInTablet(string tablet_id, MonoDelta delta).
Or maybe even:
AdvanceClockInTabletHostingKey(int key, MonoDelta delta)
This would get rid of a lot of the boiler plate in this test.
PS3, Line 479: snapshot scan
snapshot scan's timestamp
PS3, Line 498: ince the snapshot timestamp
: // is taken from the second server's clock
why is it taken from the second server? isn't it that you now have advanced the
first server's clock past all writes?
PS3, Line 507: ASSERT_LE(0, t_diff.ToMicroseconds());
same wei
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has submitted this change and it was merged. Change subject: KUDU-1679 Propagate timestamps for scans .. KUDU-1679 Propagate timestamps for scans Added the 'propagated_timestamp' field into the ScanResponsePB message. It's always set by the tablet server which processed the incoming NewScanRequestPB message successfully. Also, added a unit test to verify the presence of the field in tablet server response messages. Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Reviewed-on: http://gerrit.cloudera.org:8080/5099 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/client/client-test.cc M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scanner-internal.cc M src/kudu/integration-tests/consistency-itest.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver.proto 9 files changed, 204 insertions(+), 57 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1679 Propagate timestamps for scans .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [integration tests] added scan consistency test
David Ribeiro Alves has posted comments on this change. Change subject: [integration tests] added scan consistency test .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5084 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [integration tests] added scan consistency test
David Ribeiro Alves has posted comments on this change. Change subject: [integration tests] added scan consistency test .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/5084/4/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: Line 343: const size_t total_rows = 2U * key_split_value_; > warning: either cast from 'unsigned int' to 'const size_t' (aka 'const unsi tidy bot still complaining :P maybe avoid using size_t? -- To view, visit http://gerrit.cloudera.org:8080/5084 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-1189 if not set, use timestamp from first server
David Ribeiro Alves has posted comments on this change. Change subject: WIP: KUDU-1189 if not set, use timestamp from first server .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/5143/1/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: PS1, Line 408: configuration_.snapshot_timestamp() != ScanConfiguration::kNoTimestam maybe add a has_snapshot_timestamp() to scan_configuration? PS1, Line 408: configuration_.snapshot_timestamp() != ScanConfiguration::kNoTimestamp in general, does this cover the case where the user didn't specify a timestamp but the first server that was hit chose one for the scan? That _is_ the core thing we're trying to implement/test here, IIRC in the case where the user does specify a timestamp we already set it in all the scans. PS1, Line 409: last_response_.has_snap_timestamp() when would this not be set? Not sure what to do there: maybe crash? maybe return an error? in any case this is something that has bene in the server for quite a while and should be compatible with the foreseeable past client/server versions. -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore
David Ribeiro Alves has posted comments on this change. Change subject: [Timestamp] use 'operator<' instead of ComesBefore .. Patch Set 5: Code-Review+2 lgtm, since the tidy bot is nt your fault I'm merging this. -- To view, visit http://gerrit.cloudera.org:8080/5096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore
David Ribeiro Alves has submitted this change and it was merged.
Change subject: [Timestamp] use 'operator<' instead of ComesBefore
..
[Timestamp] use 'operator<' instead of ComesBefore
Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Besides, this patch contains a minor clean-up on the of header files
and forward declarations.
This patch does not contain any functional changes.
Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Reviewed-on: http://gerrit.cloudera.org:8080/5096
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
9 files changed, 75 insertions(+), 54 deletions(-)
Approvals:
David Ribeiro Alves: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore
David Ribeiro Alves has posted comments on this change.
Change subject: [Timestamp] use 'operator<' instead of ComesBefore
..
Patch Set 3:
(4 comments)
lgtm, but please remove the CompactionInputRow stuff
http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:
PS3, Line 474:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
:
same
PS3, Line 350: if (smallest < state->next()) {
same
PS3, Line 565: namespace {
: void AdvanceToLastInList(const Mutation** m) {
: const Mutation* next;
: while ((next = (*m)->acquire_next()) != nullptr) {
: *m = next;
: }
: }
: } // anonymous namespace
:
: // Compare the mutations of two duplicated rows.
: // Return 'true' if latest_version(left) < latest_version(right)
: bool operator<(const CompactionInputRow& lhs, const
CompactionInputRow& rhs) {
: const Mutation* left_latest = lhs.redo_head;
: const Mutation* right_latest = rhs.redo_head;
: if (right_latest == nullptr) {
: DCHECK(left_latest != nullptr);
: return true;
: }
: if (left_latest == nullptr) {
: // left must still be alive
: DCHECK(right_latest != nullptr);
: return false;
: }
:
: // Duplicated rows have disjoint histories, we don't need to
get the latest
: // mutation, the first one should be enough for the sake of
determining the most recent
: // row, but in debug mode do get the latest to make sure one
of the rows is a ghost.
: const bool ret = left_latest->timestamp() <
right_latest->timestamp();
: #ifndef NDEBUG
: AdvanceToLastInList(&left_latest);
: AdvanceToLastInList(&right_latest);
: if (left_latest->timestamp() != right_latest->timestamp()) {
: // If in fact both rows were deleted at the same time, this
is OK -- we could
: // have a case like TestRandomAccess.TestFuzz3, in which a
single batch
: // DELETED from the DRS, INSERTed into MRS, and DELETED from
MRS. In that case,
: // the timestamp of the last REDO will be the same and we
can pick whichever
: // we like.
: const bool debug_ret = left_latest->timestamp() <
right_latest->timestamp();
: CHECK_EQ(ret, debug_ret);
: }
: #endif
: return ret;
: }
same
http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:
PS3, Line 167: bool operator<(const CompactionInputRow& lhs, const
CompactionInputRow& rhs);
this seems out of place in this patch (the commit message only mentions
Timestamp) and actually goes against the reinsert stuff I have in-flight where
I completely change the comparison between two CompactionInputRows, mind
removing this from this patch?
--
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1679 Propagate timestamps for scans .. Patch Set 5: Could you put this patch after the consistency itest patch in the git history and enable that test here? -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has posted comments on this change.
Change subject: KUDU-1679 Propagate timestamps for scans
..
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5099/5//COMMIT_MSG
Commit Message:
PS5, Line 10: set by the tablet server
s/set by the tablet server/set by the tablet server to the current clock value
when a NewScanRequestPB message is processed successfully.
http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS5, Line 594: CountRows
CountRows->CountRowsOnLeaders?
http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
PS5, Line 217: READ_LATEST
s/READ_LATEST/not in READ_AT_SNAPSHOT
http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:
PS5, Line 295: if (configuration_.has_snapshot_timestamp()) {
: if (PREDICT_TRUE(read_mode ==
KuduScanner::READ_AT_SNAPSHOT)) {
:
scan->set_snap_timestamp(configuration_.snapshot_timestamp());
: } else {
: LOG(WARNING) << "Ignoring snapshot timestamp since in
READ_LATEST mode.";
: }
: }
maybe do this within the switch block above?
--
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1679 Propagate timestamps for scans .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: PS4, Line 166: uint64_t snapshot_timestamp_; > For C++ mapping the PB field is uint64: good point. dunno why I thought it mapped to int64 -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [integration tests] added scan consistency test
David Ribeiro Alves has posted comments on this change.
Change subject: [integration tests] added scan consistency test
..
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.cc
File src/kudu/client/client-test-util.cc:
Line 21:
extra line and include order
Line 78: row_count_status = scanner->Open();
if scanner->NextBatch() on LOC 89 timesout won't you Open() a scanner that's
already open here?
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:
PS2, Line 55: CountTableRows
Maybe call this CountTableRowsWithRetries or something? I would also not be
averse to making this one the only version.
http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:
PS2, Line 90: FLAGS_heartbeat_interval_ms = 10;
why?
PS2, Line 130: unique_ptr BuildTestRow(KuduTable* table, int
index) {
: unique_ptr insert(table->NewInsert());
: KuduPartialRow* row = insert->mutable_row();
: CHECK_OK(row->SetInt32(0, index));
: CHECK_OK(row->SetInt32(1, index * 2));
: CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello
%d", index;
: return insert;
: }
:
: // Inserts given number of tests rows into the default test
table
: // in the context of the specified session.
: Status InsertTestRows(KuduClient* client, KuduTable* table,
: int num_rows, int first_row = 0) {
: shared_ptr session = client->NewSession();
:
RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
: session->SetTimeoutMillis(6);
: for (int i = first_row; i < num_rows + first_row; ++i) {
: unique_ptr insert(BuildTestRow(table, i));
: RETURN_NOT_OK(session->Apply(insert.release()));
: }
: RETURN_NOT_OK(session->Flush());
: return Status::OK();
: }
:
: Status GetRowCount(KuduTable* table, KuduScanner::ReadMode
read_mode,
: uint64_t ts, size_t* row_count) {
: KuduScanner scanner(table);
: RETURN_NOT_OK(scanner.SetReadMode(read_mode));
: if (read_mode == KuduScanner::READ_AT_SNAPSHOT && ts != 0) {
: RETURN_NOT_OK(scanner.SetSnapshotRaw(ts + 1));
: }
: RETURN_NOT_OK(CountTableRows(&scanner, row_count));
: return Status::OK();
: }
:
: Status GetTabletIdForKeyValue(int32_t key_value_begin,
: int32_t key_value_end,
: const string& table_name,
: vector* tablet_ids) {
: if (!tablet_ids) {
: return Status::InvalidArgument("null output container");
: }
: tablet_ids->clear();
:
: // Find the tablet for the first range (i.e. for the rows to
be inserted).
: unique_ptr split_row_start(schema_.NewRow());
: RETURN_NOT_OK(split_row_start->SetInt32(0, key_value_begin));
: string partition_key_start;
:
RETURN_NOT_OK(split_row_start->EncodeRowKey(&partition_key_start));
:
: unique_ptr split_row_end(schema_.NewRow());
: RETURN_NOT_OK(split_row_end->SetInt32(0, key_value_end));
: string partition_key_end;
:
RETURN_NOT_OK(split_row_end->EncodeRowKey(&partition_key_end));
:
: GetTableLocationsRequestPB req;
: req.mutable_table()->set_table_name(table_name);
: req.set_partition_key_start(partition_key_start);
: req.set_partition_key_end(partition_key_end);
: master::CatalogManager* catalog =
: cluster_->mini_master()->master()->catalog_manager();
: GetTableLocationsResponsePB resp;
: CatalogManager::ScopedLeaderSharedLock l(catalog);
: RETURN_NOT_OK(l.first_failed_status());
: RETURN_NOT_OK(catalog->GetTableLocations(&req, &resp));
: for (size_t i = 0; i < resp.tablet_locations_size(); ++i) {
:
tablet_ids->emplace_back(resp.tablet_locations(i).tablet_id());
: }
:
: return Status::OK();
: }
any way to re-use so
[kudu-CR](gh-pages) Fix list rendering of the last weekly update post
David Ribeiro Alves has submitted this change and it was merged. Change subject: Fix list rendering of the last weekly update post .. Fix list rendering of the last weekly update post The sublist after "Noteworthy features/improvements" is not rendering properly. Apparently it was missing a new line to be indentified as a list but adding it made things look weird too (a new line before the list but none after). This makes the list a "definition list" which boldens the title and avoids the new line weirdness at the cost of extra indenting. Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 Reviewed-on: http://gerrit.cloudera.org:8080/5131 Reviewed-by: Todd Lipcon Tested-by: David Ribeiro Alves --- M _posts/2016-11-15-weekly-update.md 1 file changed, 2 insertions(+), 2 deletions(-) Approvals: David Ribeiro Alves: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/5131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon
[kudu-CR](gh-pages) Fix list rendering of the last weekly update post
David Ribeiro Alves has posted comments on this change. Change subject: Fix list rendering of the last weekly update post .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/5131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR](gh-pages) Fix list rendering of the last weekly update post
David Ribeiro Alves has uploaded a new patch set (#2). Change subject: Fix list rendering of the last weekly update post .. Fix list rendering of the last weekly update post The sublist after "Noteworthy features/improvements" is not rendering properly. Apparently it was missing a new line to be indentified as a list but adding it made things look weird too (a new line before the list but none after). This makes the list a "definition list" which boldens the title and avoids the new line weirdness at the cost of extra indenting. Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 --- M _posts/2016-11-15-weekly-update.md 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/5131/2 -- To view, visit http://gerrit.cloudera.org:8080/5131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves
[kudu-CR](gh-pages) Fix list rendering of the last weekly update post
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/5131 Change subject: Fix list rendering of the last weekly update post .. Fix list rendering of the last weekly update post The sublist after "Noteworthy features/improvements" is not rendering properly. Apparently it was missing a new line to be indentified as a list but adding it made things look weird too (a new line before the list but none after). This makes the list a "definition list" which boldens the title and avoids the new line weirdness at the cost of extra indenting. Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 --- M _posts/2016-11-15-weekly-update.md 1 file changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/5131/1 -- To view, visit http://gerrit.cloudera.org:8080/5131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves
[kudu-CR] [consensus] KUDU-1718: Fix few bugs around replica eviction failures
David Ribeiro Alves has posted comments on this change.
Change subject: [consensus] KUDU-1718: Fix few bugs around replica eviction
failures
..
Patch Set 1:
(1 comment)
would it be possible to split this patch into several ones that take care of
each ticker individually or would that be too hard?
http://gerrit.cloudera.org:8080/#/c/5111/1/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
PS1, Line 235: if (!controller_.status().ok()) {
: if (controller_.status().IsRemoteError()) {
: // Most controller errors are caused by network issues or
corner cases
: // like shutdown and failure to serialize a protobuf.
Therefore, we
: // generally consider these errors to indicate an
unreachable peer.
: // However, a RemoteError wraps some other error
propagated from the
: // remote peer, so we know the remote is alive. Therefore,
we will let
: // the queue know that the remote is responsive.
:
queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid());
: }
: ProcessResponseError(controller_.status());
: return;
: }
: // Again, let the queue know that the remote is still
responsive, since we
: // will not be sending this error response through to the
queue.
: // For certain error codes, we want the queue to treat the
remote as
: // unresponsive and take necessary actions, hence bypassing
the notification
: // for those error codes.
: if (response_.has_error()) {
: if (response_.error().code() !=
TabletServerErrorPB::TABLET_NOT_FOUND &&
: response_.error().code() !=
TabletServerErrorPB::WRONG_SERVER_UUID &&
: response_.error().code() !=
TabletServerErrorPB::TABLET_NOT_RUNNING) {
:
queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid());
:
ProcessResponseError(StatusFromPB(response_.error().status()));
: return;
: }
: } else if (response_.status().has_error()) {
: if (response_.status().error().code() ==
consensus::ConsensusErrorPB::CANNOT_PREPARE) {
:
queue_->NotifyPeerIsResponsiveDespiteError(peer_pb_.permanent_uuid());
:
ProcessResponseError(StatusFromPB(response_.error().status()));
: return;
: }
: }
It would be good if we could have something like we have in the c++ client
where we analize the respose and then get a directive back.
Something like:
PeerResponseStatus AnalyzeResponse() which would have the nightmarish string of
ifs inside and return:
OK
ERROR
ERROR_BUT_RESPONSIVE
would that make sense?
--
To view, visit http://gerrit.cloudera.org:8080/5111
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d3f84fd297a8e4760208a213c1ee393e92499a3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1679 Propagate timestamps for scans
David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1679 Propagate timestamps for scans .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/5099/4/python/kudu/tests/test_scanner.py File python/kudu/tests/test_scanner.py: Line 194: .set_selection(kudu.LEADER_ONLY)\ were the python tests failing after this change? http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: PS4, Line 166: uint64_t snapshot_timestamp_; why the change to uint? isn't the PB field an int64? http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: PS4, Line 209: PREDICT_TRUE(configuration_.read_mode() == KuduScanner::READ_LATEST why are we setting a snap timestamp on a READ_LATEST scan? Also this seems to go against the warning below. Did you mean to use READ_AT_SNAPSHOT here. Finally can we leave the scan token changes to another patch? this is a separate matter and I was pondering with MJ whether it was worth to keep the timestamp inside the scan token itself. http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: PS4, Line 415: snapshoty typo PS4, Line 414: // TODO(aserbin): clarify whether it's OK to override the explicitly set : // snapshoty timestamp with the timestamp from the last response. this is valid. This basically makes retries of scans use the scan of the last response. It's not supposed to be overriding anything. If the user did provide a timestamp then 'snap_timestamp' on the response should be that timestmap. If the user didn't provide one then it should be the timestamp chosen by the server. In either case we're not overriding anything by setting it here. http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS4, Line 1156: // make sure that the snapshot timestamp that was selected is >= now can you additionally make sure that the propagated timestamp is after the scan's timestamp? http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: PS4, Line 351: nder the hood, : // the client is supposed to copy the value from this field into the : // WriteRequestPB::propagated_timestamp and/or : // NewScanRequestPB::propagated_timestamp fields to achieve consistency in its : // read- and write-operations. this comment goes a bit too much into the client impl, IMO. Maybe mention this where we already introduce other consistency stuff. I think we talk a bit about it in the ReadModes definition. -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [hybrid clock] update on NowWithError's signature
David Ribeiro Alves has posted comments on this change. Change subject: [hybrid_clock] update on NowWithError's signature .. Patch Set 1: what is the goal here? we usually frown on optional args in outside of tests without a good reason, and this adds an extra branch... it also seems a bit silly from a public api prespective to call NowWithError and then not want the error back, there's Now for that. I do see that we ignore it internally but that's more because of internal reuse. -- To view, visit http://gerrit.cloudera.org:8080/5108 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia64eb9502c7fc4c10007f4f88f3346394fbdd50e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has posted comments on this change. Change subject: Add 11/15 weekly update .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has submitted this change and it was merged. Change subject: Add 11/15 weekly update .. Add 11/15 weekly update Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Reviewed-on: http://gerrit.cloudera.org:8080/5098 Reviewed-by: Todd Lipcon Tested-by: David Ribeiro Alves --- A _posts/2016-11-15-weekly-update.md 1 file changed, 86 insertions(+), 0 deletions(-) Approvals: David Ribeiro Alves: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon
[kudu-CR](gh-pages) Add 11/15 weekly update
Hello Dinesh Bhat, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5098 to look at the new patch set (#4). Change subject: Add 11/15 weekly update .. Add 11/15 weekly update Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f --- A _posts/2016-11-15-weekly-update.md 1 file changed, 86 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5098/4 -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has posted comments on this change. Change subject: Add 11/15 weekly update .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/5098/3/_posts/2016-11-15-weekly-update.md File _posts/2016-11-15-weekly-update.md: PS3, Line 16: c++ and j > nit: capitlaize C++ and Java Done PS3, Line 20: ra > Raft Done Line 32: new committer and PMC member. > voted in as a? Done PS3, Line 70: client > s/client/manual recovery/ ? Done -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has uploaded a new patch set (#3). Change subject: Add 11/15 weekly update .. Add 11/15 weekly update Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f --- A _posts/2016-11-15-weekly-update.md 1 file changed, 86 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5098/3 -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [c++client] propagating timestamp for scans
David Ribeiro Alves has posted comments on this change. Change subject: [c++client] propagating timestamp for scans .. Patch Set 1: I mention this yesterday, but it was pretty late and maybe you didn't get a chance to read it. A snapshot's timestamp is distinct from the timestamp that is propagated. If you look at NewScanRequestPB (https://github.com/cloudera/kudu/blob/master/src/kudu/tserver/tserver.proto#L209) you can see that there is a snap_timestamp field and propagated_timestamp field. We already propagate the client's timestamp to the server (when we set 'snap_timestamp' in NewScanRequestPB, what we're missing is getting a 'propagated_timestamp' in ScanResponsePB and using that to update the client's last known timestamp. The snap_timestamp in the ScanRequest/Response refers to the timestamp of the scan itself. For instance the user can set this arbitrarily in the past to do historic scans, but the propagated_timestamp would still be the current value of the servers clock. I see how having a 'snap_timestamp' in the response might be confusing, but it does refer to the scan's timestamp and not to the timestamp that must be propagated. It's there because if the user doesn't specify a timestamp for the scan the server chooses one (based on the value obtained from the clock if after being updated with the ScanRequest's propagated timestamp). The value is returned on the response so that the client can continue the scan on another tablet while using the same timestamp. -- To view, visit http://gerrit.cloudera.org:8080/5099 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has uploaded a new patch set (#2). Change subject: Add 11/15 weekly update .. Add 11/15 weekly update Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f --- A _posts/2016-11-15-weekly-update.md 1 file changed, 86 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5098/2 -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves
[kudu-CR](gh-pages) Add 11/15 weekly update
David Ribeiro Alves has uploaded a new change for review. http://gerrit.cloudera.org:8080/5098 Change subject: Add 11/15 weekly update .. Add 11/15 weekly update Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f --- A _posts/2016-11-15-weekly-update.md 1 file changed, 86 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/5098/1 -- To view, visit http://gerrit.cloudera.org:8080/5098 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I487b092739b3f921e3ab0b186f82c7f368d84f4f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-Owner: David Ribeiro Alves
[kudu-CR] WIP: [integration tests] scan inconsistency test
David Ribeiro Alves has posted comments on this change.
Change subject: WIP: [integration tests] scan inconsistency test
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5084/1/src/kudu/integration-tests/consistent-scan-test.cc
File src/kudu/integration-tests/consistent-scan-test.cc:
PS1, Line 48: class ConsistentScanTest : public KuduTest {
It would be great to have a way to have each specific test start a cluster a
certain way. For certain tests multiple tablets of one replica are ideal, for
other we need a single tablet with multiple replicas. A helper method like
StartCluster(int num_tablets, int num_replicas) would be helpful.
PS1, Line 48: ConsistentScanTest
Like we had discussed yesterday and like Todd suggested ConsistencyITest would
be a better name for the class/test.
PS1, Line 80: ExternalMiniClusterOptions
In most of the cases I can think of we need a MiniCluster, not an
ExternalMIniCluster, so that we can skew the clock. Maybe worth pondering if
we'll need a test that stops/kills servers in which case having the option to
choose between the two would be best.
PS1, Line 84: // Creates a table with the specified name and replication factor.
: void CreateTable(const string& table_name, int num_replicas,
:shared_ptr* table) {
: // Using range partitions with high split value for the key
-- this is
: // to keep the contents of the table primarily at one tablet
server.
: unique_ptr split_row(schema_.NewRow());
: ASSERT_OK(split_row->SetInt32(0, 8));
:
: unique_ptr
table_creator(client_->NewTableCreator());
: ASSERT_OK(table_creator->table_name(table_name)
: .schema(&schema_)
: .add_range_partition_split(split_row.release())
: .set_range_partition_columns({ key_column_name_ })
: .num_replicas(num_replicas)
: .Create());
: ASSERT_OK(client_->OpenTable(table_name, table));
: }
Worth looking into other itests to see if we can reuse some of the
setup/helpers there. TabletHistoryGcITest or RaftConsensusITest come to mind
PS1, Line 102: unique_ptr BuildTestRow(KuduTable* table, int index)
{
: unique_ptr insert(table->NewInsert());
: KuduPartialRow* row = insert->mutable_row();
: CHECK_OK(row->SetInt32(0, index));
: CHECK_OK(row->SetInt32(1, index * 2));
: CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello
%d", index;
: return insert;
: }
:
: // Inserts given number of tests rows into the default test
table
: // in the context of the specified session.
: Status InsertTestRows(KuduSession* session, InsertFlushOptions
flush_opt,
: int num_rows, int first_row = 0) {
:
RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
: session->SetTimeoutMillis(6);
: for (int i = first_row; i < num_rows + first_row; ++i) {
: unique_ptr insert(BuildTestRow(table_.get(),
i));
: RETURN_NOT_OK(session->Apply(insert.release()));
: }
: if (flush_opt == OPT_FLUSH) {
: RETURN_NOT_OK(session->Flush());
: }
: return Status::OK();
: }
:
: Status GetRowCount(KuduClient::ReplicaSelection
replica_selection,
: KuduScanner::ReadMode read_mode,
: size_t* row_count) {
: KuduScanner scanner(table_.get());
: RETURN_NOT_OK(scanner.SetReadMode(read_mode));
: RETURN_NOT_OK(scanner.SetSelection(replica_selection));
: // KUDU-1656: there might be timeouts, so re-try the
operations to
: // avoid test flakiness.
: Status row_count_status;
: size_t actual_row_count = 0;
: for (size_t i = 0; i < 3; ++i) {
: row_count_status = scanner.Open();
: if (!row_count_status.ok()) {
: if (row_count_status.IsTimedOut()) {
: // Start the row count over again.
: continue;
: }
: RETURN_NOT_OK(row_count_status);
: }
: size_t count = 0;
: while (scanner.HasMoreRows()) {
: KuduScanBatch batch;
: row_count_status = scanner.NextBatch(&batch);
[kudu-CR] KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5057 to look at the new patch set (#5). Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc .. KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc This completely removes that APIs that allow automatic timestamp assignment and safe time adjustment from Mvcc. Previous patches had taken care of making sure these were unused in regular TabletServer operations or in tests that use LocalTabletWriter. This patch takes this the rest of the way by removing the APIs from Mvcc completely and changing tests that use it directly to obtain the timestamps from a clock. Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/transactions/transaction_driver.cc 11 files changed, 137 insertions(+), 217 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/5 -- To view, visit http://gerrit.cloudera.org:8080/5057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5057 to look at the new patch set (#4). Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc .. KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc This completely removes that APIs that allow automatic timestamp assignment and safe time adjustment from Mvcc. Previous patches had taken care of making sure these were unused in regular TabletServer operations or in tests that use LocalTabletWriter. This patch takes this the rest of the way by removing the APIs from Mvcc completely and changing tests that use it directly to obtain the timestamps from a clock. Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/transactions/transaction_driver.cc 11 files changed, 134 insertions(+), 214 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/4 -- To view, visit http://gerrit.cloudera.org:8080/5057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5057 to look at the new patch set (#3). Change subject: KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc .. KUDU-798 (part 3) - Remove automatic safe time adjustment from mvcc This completely removes that APIs that allow automatic timestamp assignment and safe time adjustment from Mvcc. Previous patches had taken care of making sure these were unused in regular TabletServer operations or in tests that use LocalTabletWriter. This patch takes this the rest of the way by removing the APIs from Mvcc completely and changing tests that use it directly to obtain the timestamps from a clock. Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/transactions/transaction_driver.cc 11 files changed, 133 insertions(+), 214 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/3 -- To view, visit http://gerrit.cloudera.org:8080/5057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 3) Remove automatic safe time adjustment from mvcc
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5057 to look at the new patch set (#2). Change subject: KUDU-798 (part 3) Remove automatic safe time adjustment from mvcc .. KUDU-798 (part 3) Remove automatic safe time adjustment from mvcc This completely removes that APIs that allow automatic timestamp assignment and safe time adjustment from Mvcc. Previous patches had taken care of making sure these were unused in regular TabletServer operations or in tests that use LocalTabletWriter. This patch takes this the rest of the way by removing the APIs from Mvcc completely and changing tests that use it directly to obtain the timestamps from a clock. Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 --- M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/diskrowset-test-base.h M src/kudu/tablet/diskrowset-test.cc M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/memrowset-test.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/transactions/transaction_driver.cc 11 files changed, 133 insertions(+), 214 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5057/2 -- To view, visit http://gerrit.cloudera.org:8080/5057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5056 to look at the new patch set (#3). Change subject: KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests .. KUDU-798 (part 2) - Always use pre-assigned timestamps in tablet tests This goes another step into removing safe time from mvcc. In this patch we make tablet tests always use one instance of LocalTabletWriter and use the same path as with regular transactions: i.e. always use pre-assigned timestamps. This means that tests now need to share an instance of LocalTabletWriter since timestamp assignement does not retry until it finds an unassigned timestamp anymore. Still left for a follow-up patch is the complete removal of safe time from mvcc as that requires changing the tests that interct with MvccManager directly. Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc --- M src/kudu/tablet/local_tablet_writer.h M src/kudu/tablet/mt-tablet-test.cc M src/kudu/tablet/tablet-test-base.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet_history_gc-test.cc 6 files changed, 171 insertions(+), 140 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/5056/3 -- To view, visit http://gerrit.cloudera.org:8080/5056 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic08be57ed5ab38e3980c85102971c5998f7da2dc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
