[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Reviewed-on: http://gerrit.cloudera.org:8080/8804 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy Reviewed-by: David Ribeiro Alves Reviewed-by: Alexey Serbin --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 326 insertions(+), 98 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, but someone else must approve David Ribeiro Alves: Looks good to me, approved Alexey Serbin: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 19 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 18: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726 PS16, Line 1726: > Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenari Ah, I see. Thanks for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 19:08:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 18: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 18:37:43 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 18: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 08:01:41 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 18: (2 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: > Thanks for the explanation. Let's add that explanation in the comment here, Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: > Maybe we should just add that explanation to the docs? Updated. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 05:23:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#18). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 326 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/18 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 17: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > Maybe we should just add that explanation to the docs? Thanks for the explanation. Let's add that explanation in the comment here, maybe something like: // fall into that category because calling clock->Update(propagated_timestamp) verifies that propagated_timestamp + FLAGS_max_clock_sync_error_usec is not too far into the future. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 17 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 01:36:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > As we update the clock based on propagated timestamp at L2140, if (propagat Maybe we should just add that explanation to the docs? Also: obvious while we have these things in context, but likely worth mentioning for the documentation value is that "clean" timestamp is, by definition in the past. Suggestion, something like: There is no need to validate if the chosen timestamp is too far in the future, as : - MVCC's 'clean' ts is by definition in the past (it's maximally bounded by safe time). - "propagated" ts was used to update the clock above and the update would have returned an error if the the timestamp was too far in the future. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 00:42:13 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (10 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726 PS16, Line 1726: > What happened to this test? Maybe I'm missing something, but it seems this Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenario of this test case. I removed it as it is duplicated. http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157 PS16, Line 157: TimeStamp > nit: in the code around, we have Timestamp as a type; maybe, name it consis Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161 PS16, Line 161: PickAndVerifyTimeStamp > nit: ditto, PickAndVerifyTimestamp(...) ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162 PS16, Line 162: tablet::TabletReplica* > Could it be const tablet::TabletReplica& ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757 PS16, Line 1757: case READ_YOUR_WRITES: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042 PS16, Line 2042: case READ_AT_SNAPSHOT: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117 PS16, Line 2117: . > s/./,/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: initiated > s/initiated with/default-constructed to/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: Since > s/Since/since/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > is that guaranteed by something? As we update the clock based on propagated timestamp at L2140, if (propagation ts + 1) is a timestamp too far in the future, which essentially means (propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec). -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 23:50:48 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#17). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 322 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/17 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 17 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (4 comments) lgtm, just a nit and one last question http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117 PS16, Line 2117: . s/./,/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: initiated s/initiated with/default-constructed to/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: Since s/Since/since/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not is that guaranteed by something? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 20:47:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (6 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726 PS16, Line 1726: What happened to this test? Maybe I'm missing something, but it seems this test should stay relevant regardless of the new scan mode. http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157 PS16, Line 157: TimeStamp nit: in the code around, we have Timestamp as a type; maybe, name it consistently with that so it's ValidateTimestamp(...) ? http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161 PS16, Line 161: PickAndVerifyTimeStamp nit: ditto, PickAndVerifyTimestamp(...) ? http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162 PS16, Line 162: tablet::TabletReplica* Could it be const tablet::TabletReplica& ? Or, maybe, Tablet* ? http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757 PS16, Line 1757: case READ_YOUR_WRITES: nit: per coding standard, add // fallthrough http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042 PS16, Line 2042: case READ_AT_SNAPSHOT: nit: per coding standard, add // fallthrough -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 17 Feb 2018 01:44:55 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Sat, 17 Feb 2018 00:36:50 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#16). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 321 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/16 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 14: (3 comments) http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156 PS14, Line 156: // as such timestamp is invalid. > nit: "such a" Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114 PS12, Line 2114: } : > Good question/point. I wrote that and it was cryptic to me too. I now reali I see, thanks David for the explanation. http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046 PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode specified"); > nit: I think this can FATAL out. it's us (not the user) that call this and Done -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 23:26:23 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#15). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 321 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/15 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 15 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114 PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed : // to be higher than 'tmp_snap_timestamp'. > Good question. TBH, this is refactored from previous code, so I am not sure Good question/point. I wrote that and it was cryptic to me too. I now realize that I'm just referring to the fact that if we don't set a timestamp at all, it get's set to MAX_LONG - 1 (kInvalidTimestamp). The point is to put reader at ease that even if we don't get a global latest ts from the clock, it's still safe to compare the snap timestamp below. Thanks for fixing this Hao -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 22:50:13 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 14: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166 PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); > What is this really checking. We need to validate the timestamp for READ_AT Right, think a bit more, I do not see a case where this validation could fail (which essentially mean: propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec) So it may not worth checking here. I will remove. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 22:44:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 14: (3 comments) http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156 PS14, Line 156: // as such timestamp is invalid. nit: "such a" http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046 PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode specified"); nit: I think this can FATAL out. it's us (not the user) that call this and we should only call it for "snapshotty" scans http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166 PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); What is this really checking. We need to validate the timestamp for READ_AT_SNAPSHOT as the user can send a timestamp that is arbitrarily in the future, but here it can only be either "clean_timestamp" (which is valid) or propagated timestamp, which is necessarily valid since we've udpated the clock with it in line 2139. In other words do you think that there is a test case you can write that would fail this check? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 20:08:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 14: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 18:58:54 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#14). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 320 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/14 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 14: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/13/src/kudu/tserver/tablet_server-test-base.cc File src/kudu/tserver/tablet_server-test-base.cc: http://gerrit.cloudera.org:8080/#/c/8804/13/src/kudu/tserver/tablet_server-test-base.cc@423 PS13, Line 423: ScanRequestPB req; > warning: parameter 'read_mode' is unused [misc-unused-parameters] Done -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 16 Feb 2018 00:00:24 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#13). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 353 insertions(+), 107 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/13 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 12: (23 comments) http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG@15 PS12, Line 15: choose the newest timestamp : within the staleness bound that allows execution of the reads without : being blocked by the in-flight transactions > might be worth adding "if possible" Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@236 PS12, Line 236: subject to the propagated timestamp bound > This wording is pretty hard to a casual reader (or even me) to understand. Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@237 PS12, Line 237: newest timestamp within > latest timestamp higher than Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@242 PS12, Line 242: the timestamp > a timestamp Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@244 PS12, Line 244: should use it > Is this something that end-users have to worry about, or can we word this s Updated. Currently, we use current clock time of the server as the propagation time. This would bump up the propagation time unnecessarily for the next read in this mode. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@245 PS12, Line 245: wait > nit: waiting Done http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc@185 PS11, Line 185: void ScanYourWritesTest(uint64_t propagated_timestamp, ScanResponsePB* resp); > warning: parameter 'propagated_timestamp' is const-qualified in the functio Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS12: > We are missing a C++ client test in this patch. Is that excluded on purpose C++ client test in the following CR https://gerrit.cloudera.org/#/c/8823/ http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) { > nit: add a comment at the top of this test describing what the test does Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites > It worries me a bit that most tests of this functionality overwhelmingly te Added more integration tests in C++ client which would definitely fail with READ_LATEST. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites > I agree that a deterministic test would be nice and I think it should be po Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1927 PS12, Line 1927: e > nit: missing period Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1958 PS12, Line 1958: TEST_F(TabletServerTest, TestScanYourWrites_WithoutPropagatedTimestamp) { > nit: add test comment Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1960 PS12, Line 1960: perform a write > nit: use capitalization and punctuation for code comments per https://googl Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1972 PS12, Line 1972: perform a write > punctuation Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@155 PS12, Line 155: // it exceeds the maximum allowed clock synchronization error time. > Can we add a note about the context, i.e. why this matters? Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@158 PS12, Line 158: if > s/if/that/ Done http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2113 PS12, Line 2113: > add: DCHECK(s.ok()) ? I think even for status that returns 'NotSupported' can reach here. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114 PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed : // to be higher than 'tmp_snap_timestamp'. > I'm having trouble following this: Good question. TBH, this is refactored from previous code, so I am not sure about th
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 12: (20 comments) http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@236 PS12, Line 236: subject to the propagated timestamp bound This wording is pretty hard to a casual reader (or even me) to understand. I think it would be more clear to say something like: Each tablet server will choose a timestamp to use for a server-local snapshot scan subject to the following criteria: (1) It will be higher than the propagated timestamp, (2) It will attempt to minimize latency caused by waiting for outstanding write transactions to complete. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@237 PS12, Line 237: newest timestamp within latest timestamp higher than http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@242 PS12, Line 242: the timestamp a timestamp http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@244 PS12, Line 244: should use it Is this something that end-users have to worry about, or can we word this so that it sounds like Kudu will take care of it? If the latter, how about "The Kudu client library will use it as the propagated timestamp for subsequent reads" or something along those lines? However I'm not sure why we are making the distinction here between a normally propagated timestamp and this different thing. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@245 PS12, Line 245: wait nit: waiting http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS12: We are missing a C++ client test in this patch. Is that excluded on purpose? http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites > It worries me a bit that most tests of this functionality overwhelmingly te I agree that a deterministic test would be nice and I think it should be possible using two clients. It would also be nice to have a non-deterministic test where you have two clients in RYW mode reading and scanning. Loop them concurrently in separate threads at least a certain number of rounds or amount of time, as well as until both of the following conditions hold, while also triggering leader elections to make it easier to hit anomalies: 1. neither has ever violated RYW locally 2. we have observed anomalies where if we had been using a strict serializable approach then certain writes would have appeared, but they don't because of the hidden channel (the other client). http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) { nit: add a comment at the top of this test describing what the test does http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1927 PS12, Line 1927: e nit: missing period http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1958 PS12, Line 1958: TEST_F(TabletServerTest, TestScanYourWrites_WithoutPropagatedTimestamp) { nit: add test comment http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1960 PS12, Line 1960: perform a write nit: use capitalization and punctuation for code comments per https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1972 PS12, Line 1972: perform a write punctuation http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@155 PS12, Line 155: // it exceeds the maximum allowed clock synchronization error time. Can we add a note about the context, i.e. why this matters? nit: Also perhaps rename to ValidateSnapshotTimestamp() ? Usually Stamp in Timestamp is not capitalized in the Kudu code, and this seems specific to snapshot timestamps. http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@158 PS12, Line 158: if s/if/that/ http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2113 PS12, Line 2113: add: DCHECK(s.ok()) ? http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114 PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed : // to be higher than 'tmp_snap
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG@15 PS12, Line 15: choose the newest timestamp : within the staleness bound that allows execution of the reads without : being blocked by the in-flight transactions might be worth adding "if possible" http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924 PS12, Line 1924: TestScanYourWrites It worries me a bit that most tests of this functionality overwhelmingly tests the "happy" case. A good test of that is: if you changed the mode of all tests here to READ_LATEST would it fail? if not that's ok as long as we have some test somewhere that would, however I didn't see such a tests even after the client implementations. Jepsen tests are good to cover more possibilities, but some deterministic tests just for RYW should exist. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 09 Feb 2018 20:09:16 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#12). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 4 files changed, 234 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/12 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222 PS10, Line 222: tion is > nit: snapshot timestamp Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228 PS10, Line 228: > nit: choose Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229 PS10, Line 229: serv > nit: such that Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231 PS10, Line 231: > The 'stale' terminology hasn't been introduced here, so I think it would be Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233 PS10, Line 233: > I think it would be better to say 'different results' here in order to avoi Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922 PS10, Line 1922: > We decided we also wanted scan your reads right? is that covered? Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944 PS10, Line 1944: ASSERT_EQ(R"((in > I know this is likely copy/paste but remove (adds nothing) Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953 PS10, Line 1953: ASSERT_EQ(kNumRows, results.size()); : ASSERT_EQ(R"((int32 key=0, int32 int_val > I think we discussed a couple of reasons why it might be interesting to hav Right, updated it accordingly. http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989 PS10, Line 1989: Hybrid > same Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158 PS10, Line 158: ing to the scan m > on the read mode? Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107 PS10, Line 2107: us s = server_->cl > nit add a predict false Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161 PS10, Line 2161: ) > use kMinTimestamp or something? I understand you can't use kInvalidTimestam Done http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163 PS10, Line 2163: Timestamp(std::max(propagated_timestamp + 1, clean_timestamp)); : RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); > std:max ? Done -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 08 Feb 2018 04:36:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#11). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 4 files changed, 234 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/11 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953 PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back. : ASSERT_TRUE(!resp.has_snap_timestamp()); > I see, so looks like you are proposing to make this read mode fault-toleran I think we discussed a couple of reasons why it might be interesting to have the response include the timestamp (like using that to update the last propagated timestamp). I agree that fault-tolerance work doesn't need to be included in this patch though -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 06 Feb 2018 23:50:05 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953 PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back. : ASSERT_TRUE(!resp.has_snap_timestamp()); > I'm still unsure why we wouldn't the response to include the timestamp. I a I see, so looks like you are proposing to make this read mode fault-tolerant as well. Which means we need to make it an ordered scan and ensure the client sent out the last primary key to retry on. I do not see any reasons to not make this mode fault tolerant, but I do not see the reasons why we have to return the chosen snapshot timestamp for the scan to retry on another server in this case. I think the next tsever that the scan is retried on can chose the timestamp freely based on the conditions, right? Moreover, would you mind I do another patch if we do want RYW to be fault tolerant, to make it more clear? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 06 Feb 2018 01:21:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237 PS10, Line 237: READ_YOUR_WRITES = 3; > It may be good to add docs about whether a snapshot timestamp and propagate Will update. We do not return the chosen snapshot timestamp, because as we discussed offline, for multi tablet scan we should not reuse the timestamps across servers in this mode. Each server should be free to choose a timestamp as long as it respects the aforementioned condition (This is documented in the design doc as well :)). The returned propagated timestamp is set to 'Now' of the tserver (same as other scan mode). However, as discussed the client should only update the propagated timestamp after the scan of last tablet for multi tablet scan. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 02 Feb 2018 22:49:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222 PS10, Line 222: snapshot nit: snapshot timestamp http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228 PS10, Line 228: take nit: choose http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229 PS10, Line 229: which nit: such that http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922 PS10, Line 1922: TestScanYourWrites We decided we also wanted scan your reads right? is that covered? http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944 PS10, Line 1944: // Send the call I know this is likely copy/paste but remove (adds nothing) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953 PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back. : ASSERT_TRUE(!resp.has_snap_timestamp()); I'm still unsure why we wouldn't the response to include the timestamp. I agree that the scan itself (seen as a collection os scans to multiple tablets) can't be described by a single timestamp, but having the timestamp in the response would allows to have fault-tolerant scans, right? http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989 PS10, Line 1989: // Send the call same http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158 PS10, Line 158: on the read mode, on the read mode? http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107 PS10, Line 2107: s.IsNotSupported() nit add a predict false http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161 PS10, Line 2161: 0 use kMinTimestamp or something? I understand you can't use kInvalidTimestamp (it's big) but don't want to accidentally set time to 0 (been there done that :)) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163 PS10, Line 2163: tmp_snap_timestamp > clean_timestamp ? : tmp_snap_timestamp : clean_timestamp; std:max ? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 02 Feb 2018 22:33:39 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231 PS10, Line 231: stale The 'stale' terminology hasn't been introduced here, so I think it would be best to avoid it. Perhaps 'two READ_YOUR_WRITES scans, ...' http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233 PS10, Line 233: inconsistent I think it would be better to say 'different results' here in order to avoid confusion over the formal consistency guarantees. http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237 PS10, Line 237: READ_YOUR_WRITES = 3; It may be good to add docs about whether a snapshot timestamp and propagated timestamp are returned, and what the client should do with them. I'm surprised to see in the latest revision that READ_YOUR_WRITES doesn't return a snapshot timestamp, why is that? And is the returned propagated timestamp set to the snapshot timestamp of the scan? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 02 Feb 2018 17:17:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227 PS7, Line 227: som > might be more clear to replace 'any' with 'some' here. Done http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227 PS7, Line 227: > in Done http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@231 PS7, Line 231: Reads in this mode are not repeatable: two stale reads, even if they : // provide the same propaga > I think it's worth going into a little more detail - READ_YOUR_WRITES is by Hmm, I actually updated this in the latest patch (patch 9). Sorry for any confusion. http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@233 PS7, Line 233: mps an > s/picked/chosen Done http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h@154 PS7, Line 154: so far in the future that : // it exce > 'not so far in the future that it exceeds' Done http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc@1625 PS7, Line 1625: t > not your fault, but this looks like an extraneous quote. Done -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 01 Feb 2018 22:45:10 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#10). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions. READ_OWN_WRITES mode is not repeatable. However, it allows client local read-your-writes. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 4 files changed, 260 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/10 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 7: (6 comments) Just a few nits on docs, but otherwise LGTM. http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227 PS7, Line 227: n in http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227 PS7, Line 227: any might be more clear to replace 'any' with 'some' here. http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@231 PS7, Line 231: READ_YOUR_WRITES is repeatable, i.e. all future reads at the same timestamp : // will yield the same rows I think it's worth going into a little more detail - READ_YOUR_WRITES is by itself not repeatable, but a READ_YOUR_WRITES followed by a READ_AT_SNAPSHOT with the same timestamp chosen is repeatable. http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@233 PS7, Line 233: picked s/picked/chosen http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h@154 PS7, Line 154: too far in the future that : // exceeds 'not so far in the future that it exceeds' http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc@1625 PS7, Line 1625: ' not your fault, but this looks like an extraneous quote. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Thu, 01 Feb 2018 19:41:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#9). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions. READ_OWN_WRITES mode is not repeatable. However, it allows client local read-your-writes. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 4 files changed, 260 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/9 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#8). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions. READ_OWN_WRITES mode is not repeatable. However, it allows client local read-your-writes. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 4 files changed, 260 insertions(+), 52 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/8 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy