David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add BOUNDED_READ scan mode ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@221 PS1, Line 221: When BOUNDED_READ is specified, the server will pick an arbitrarily snapshot : // in the past, subject to the propagated timestamp bound, and perform a read. : // More specifically, the server will choose the newest timestamp within the : // staleness bound that allows execution of the reads without blocking by the : // in-flight transactions. If no propagated timestamp is provided the server : // will take the timestamp which all transactions before it are committed. Not a big fan of BOUNDED_READ (bounded by what?) but I've had trouble to come up with a good name in the past. Some candidates: READ_BOUNDED_STALE - Pros: formally descriptive. Cons: has STALE in the name :); Unfamiliar READ_OWN_WRITES - Pros: familiar. Cons: People might interpret to mean ROW for all clients, while this is client local, but maybe OWN is good enough of a hint. http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228 PS1, Line 228: Boundedly stale reads are not repeatable: two stale reads, even if they : // use the same staleness bound, can execute at different timestamps and thus : // return inconsistent results. However, it allows read-your-writes for each : // client, as the picked timestamp must be higher than the one of the last : // write or read, known from the propagated timestamp. Hum, if we're waiting for a clean timestamp then the read is repeatable, right? there is no reason why we couldn't scan again at that timestamp from this or from some other client http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2036 PS1, Line 2036: ReadMode read_mode = scan_pb.read_mode(); : if (read_mode != READ_AT_SNAPSHOT && read_mode != BOUNDED_READ) { : return Status::NotSupported("Unsupported snapshot scan mode specified"); : } nit: use switch case http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2041 PS1, Line 2041: // If the client sent a timestamp update our clock with it. : if (scan_pb.has_propagated_timestamp()) { : Timestamp propagated_timestamp(scan_pb.propagated_timestamp()); : : // Update the clock so that we never generate snapshots lower that : // 'propagated_timestamp'. If 'propagated_timestamp' is lower than : // 'now' this call has no effect. If 'propagated_timestamp' is too much : // into the future this will fail and we abort. : RETURN_NOT_OK(server_->clock()->Update(propagated_timestamp)); : } : : Timestamp tmp_snap_timestamp; : Tablet* tablet = tablet_replica->tablet(); : scoped_refptr<consensus::TimeManager> time_manager = tablet_replica->time_manager(); : tablet::MvccManager* mvcc_manager = tablet->mvcc_manager(); : : if (read_mode == READ_AT_SNAPSHOT) { : // For READ_AT_SNAPSHOT mode, : // 1) if the client provided no snapshot timestamp we take the current : // clock time as the snapshot timestamp. : // 2) else we use the client provided one, but make sure it is not too : // far in the future as to be invalid. : if (!scan_pb.has_snap_timestamp()) { : tmp_snap_timestamp = server_->clock()->Now(); : } else { : tmp_snap_timestamp.FromUint64(scan_pb.snap_timestamp()); : RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); : } : } else { : // For BOUNDED_READ mode, : // 1) if the client provided no propagated timestamp we take the mvcc : // 'clean' timestamp as the snapshot timestamp. : // 2) else we use the MAX(propagated timestamp + 1, 'clean' timestamp) : // and make sure it is not too far in the future as to be invalid. : if (!scan_pb.has_propagated_timestamp()) { : tmp_snap_timestamp = mvcc_manager->GetCleanTimestamp(); : } else { : tmp_snap_timestamp.FromUint64(scan_pb.propagated_timestamp() + 1); : tmp_snap_timestamp = tmp_snap_timestamp > mvcc_manager->GetCleanTimestamp() ? : tmp_snap_timestamp : mvcc_manager->GetCleanTimestamp(); : RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp)); : } : } : : // Before we wait on anything check that the timestamp is after the AHM. : // This is not the final check. We'll check this again after the iterators are open but : // there is no point in waiting if we can't actually scan afterwards. : RETURN_NOT_OK(VerifyNotAncientHistory(tablet_replica->tablet(), : read_mode, : tmp_snap_timestamp)); Can you move all of this to a new method? maybe PickOrValidateTimestamp() http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2136 PS1, Line 2136: // Do not return the picked snapshot timestamp for BOUNDED_READ mode. why? -- 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: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 12 Dec 2017 02:04:28 +0000 Gerrit-HasComments: Yes
