You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/09/28 16:40:49 UTC
[kudu-CR] wip KUDU-2612 p13: add txn memrowsets to tablet
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16515
Change subject: wip KUDU-2612 p13: add txn memrowsets to tablet
......................................................................
wip KUDU-2612 p13: add txn memrowsets to tablet
DONT_BUILD
wip: this is the remainder of https://gerrit.cloudera.org/c/16438/
rebased; I still need to address some of the TODOs.
This patch adds a single MRS to the tablet per transaction. Currently,
these MRSs can only be flushed once their transactions are committed.
During a MRS flush, all committed MRSs are flushed together.
When inserting as a part of a transaction, in addition to the usual DRSs
and main MRS, all committed MRSs will be consulted to determine whether
the row already exists.
When iterating through rows, only committed MRSs are iterated through.
Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
---
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica-test-base.cc
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
15 files changed, 506 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/16515/1
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16515
to look at the new patch set (#4).
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
KUDU-2612: add txn memrowsets to tablet
This patch introduces the ability to insert data into per-transaction
MRSs. When a transaction is started, it creates a new MRS to which
inserts of that transaction can be applied. This "uncommitted
transactional MRS" cannot be inserted to by any other transaction and
cannot be scanned by any scanner. Once the transaction is committed,
i.e. the FINALIZE_COMMIT op is applied for the transaction, the MRS is
moved to a separate set of "committed" transactional MRSs that are
available to be scanned, updated, and flushed. If the transaction is
aborted, the transactional MRS is excised from the list of transactional
MRSs entirely.
When inserting, updating, or scanning rows, ops must now consider all
committed transactional MRSs, in addition to the main, non-transactional
MRS, checking for row presence in any store that is "committed" and
visible to users.
Additionally, when a MRS flush is performed, rather than flushing the
main, non-transactional MRS alone, Kudu will now collect the
non-transactional MRS along with all committed transactional MRSs, and
roll DRSs using the same rowset-merging code that exists for
compactions. The new flushed data will honor each transactions' commit
timestamp rather than the per-row apply timestamps. Existing MRS-related
metrics have been updated to reflect that such flush ops now need to
consider transactional MRSs.
In order to support rebuilding these MRSs when restarting, a new boolean
field is added to the transaction metadata: 'flushed_committed_mrs',
which gets set when the transactional MRS is flushed to disk. If true,
the transaction's write ops do not need to be replayed, since the
transaction does not have an active MRS. Otherwise, the transaction's
write ops do need to be replayed, even if the transaction is persisted
in metadata as being committed.
Additionally, a transaction ID is added to the MutatedStorePB message
stored in the WAL, for the sake of replaying ops that land in committed,
transactional MRSs. Upon replay of such mutations, if the transactional
MRS being mutated to is not active (i.e. it has been flushed), the
mutation op can be ignored.
When starting up, we start transactional MRSs for all transactions that
do not have 'flushed_committed_mrs' set, which indicates that the
transaction's MRS was not flushed before restarting.
NOTE: the locking story here is not complete -- multiple transactions
can write to the same rows without consequence, which violates our row
uniqueness constraints. This will be addressed in future patches.
Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica-test-base.cc
M src/kudu/tablet/txn_metadata.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.cc
M src/kudu/tablet/txn_participant.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/util/locks.h
23 files changed, 1,368 insertions(+), 238 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/16515/4
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16515
to look at the new patch set (#3).
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
KUDU-2612: add txn memrowsets to tablet
This patch introduces the ability to insert data into per-transaction
MRSs. When a transaction is started, it creates a new MRS to which
inserts of that transaction can be applied. This "uncommitted
transactional MRS" cannot be inserted to by any other transaction and
cannot be scanned by any scanner. Once the transaction is committed,
i.e. the FINALIZE_COMMIT op is applied for the transaction, the MRS is
moved to a separate set of "committed" transactional MRSs that are
available to be scanned, updated, and flushed. If the transaction is
aborted, the transactional MRS is excised from the list of transactional
MRSs entirely.
When inserting, updating, or scanning rows, ops must now consider all
committed transactional MRSs, in addition to the main, non-transactional
MRS, checking for row presence in any store that is "committed" and
visible to users.
Additionally, when a MRS flush is performed, rather than flushing the
main, non-transactional MRS alone, Kudu will now collect the
non-transactional MRS along with all committed transactional MRSs, and
roll DRSs using the same rowset-merging code that exists for
compactions. The new flushed data will honor each transactions' commit
timestamp rather than the per-row apply timestamps. Existing MRS-related
metrics have been updated to reflect that such flush ops now need to
consider transactional MRSs.
In order to support rebuilding these MRSs when restarting, a new boolean
field is added to the transaction metadata: 'flushed_committed_mrs',
which gets set when the transactional MRS is flushed to disk. If true,
the transaction's write ops do not need to be replayed, since the
transaction does not have an active MRS. Otherwise, the transaction's
write ops do need to replayed, even if the transaction is persisted in
metadata as being committed.
Additionally, a transaction ID is added to the MutatedStorePB message
stored in the WAL, for the sake of replaying ops that land in committed,
transactional MRSs. Upon replay of such mutations, if the transactional
MRS being mutated to is not active (i.e. it has been flushed), the
mutation op can be ignored.
When starting up, we start transactional MRSs for all transactions that
do not have 'flushed_committed_mrs' set, which indicates that the
transaction's MRS was not flushed before restarting.
NOTE: the locking story here is not complete -- multiple transactions
can write to the same rows without consequence, which violates our row
uniqueness constraints. This will be addressed in future patches.
Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica-test-base.cc
M src/kudu/tablet/txn_metadata.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.cc
M src/kudu/tablet/txn_participant.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
22 files changed, 1,308 insertions(+), 235 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/16515/3
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
KUDU-2612: add txn memrowsets to tablet
This patch introduces the ability to insert data into per-transaction
MRSs. When a transaction is started, it creates a new MRS to which
inserts of that transaction can be applied. This "uncommitted
transactional MRS" cannot be inserted to by any other transaction and
cannot be scanned by any scanner. Once the transaction is committed,
i.e. the FINALIZE_COMMIT op is applied for the transaction, the MRS is
moved to a separate set of "committed" transactional MRSs that are
available to be scanned, updated, and flushed. If the transaction is
aborted, the transactional MRS is excised from the list of transactional
MRSs entirely.
When inserting, updating, or scanning rows, ops must now consider all
committed transactional MRSs, in addition to the main, non-transactional
MRS, checking for row presence in any store that is "committed" and
visible to users.
Additionally, when a MRS flush is performed, rather than flushing the
main, non-transactional MRS alone, Kudu will now collect the
non-transactional MRS along with all committed transactional MRSs, and
roll DRSs using the same rowset-merging code that exists for
compactions. The new flushed data will honor each transactions' commit
timestamp rather than the per-row apply timestamps. Existing MRS-related
metrics have been updated to reflect that such flush ops now need to
consider transactional MRSs.
In order to support rebuilding these MRSs when restarting, a new boolean
field is added to the transaction metadata: 'flushed_committed_mrs',
which gets set when the transactional MRS is flushed to disk. If true,
the transaction's write ops do not need to be replayed, since the
transaction does not have an active MRS. Otherwise, the transaction's
write ops do need to be replayed, even if the transaction is persisted
in metadata as being committed.
Additionally, a transaction ID is added to the MutatedStorePB message
stored in the WAL, for the sake of replaying ops that land in committed,
transactional MRSs. Upon replay of such mutations, if the transactional
MRS being mutated to is not active (i.e. it has been flushed), the
mutation op can be ignored.
When starting up, we start transactional MRSs for all transactions that
do not have 'flushed_committed_mrs' set, which indicates that the
transaction's MRS was not flushed before restarting.
NOTE: the locking story here is not complete -- multiple transactions
can write to the same rows without consequence, which violates our row
uniqueness constraints. This will be addressed in future patches.
Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Reviewed-on: http://gerrit.cloudera.org:8080/16515
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica-test-base.cc
M src/kudu/tablet/txn_metadata.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.cc
M src/kudu/tablet/txn_participant.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
M src/kudu/util/locks.h
23 files changed, 1,368 insertions(+), 238 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16515
to look at the new patch set (#2).
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
KUDU-2612: add txn memrowsets to tablet
This patch introduces the ability to insert data into per-transaction
MRSs. When a transaction is started, it creates a new MRS to which
inserts of that transaction can be applied. This "uncommitted
transactional MRS" cannot be inserted to by any other transaction and
cannot be scanned by any scanner. Once the transaction is committed,
i.e. the FINALIZE_COMMIT op is applied for the transaction, the MRS is
moved to a separate set of "committed" transactional MRSs that are
available to be scanned, updated, and flushed. If the transaction is
aborted, the transactional MRS is excised from the list of transactional
MRSs entirely.
When inserting, updating, or scanning rows, ops must now consider all
committed transactional MRSs, in addition to the main, non-transactional
MRS, checking for row presence in any store that is "committed" and
visible to users.
Additionally, when a MRS flush is performed, rather than flushing the
main, non-transactional MRS alone, Kudu will now collect the
non-transactional MRS along with all committed transactional MRSs, and
roll DRSs using the same rowset-merging code that exists for
compactions. The new flushed data will honor each transactions' commit
timestamp rather than the per-row apply timestamps. Existing MRS-related
metrics have been updated to reflect that such flush ops now need to
consider transactional MRSs.
In order to support rebuilding these MRSs when restarting, a new boolean
field is added to the transaction metadata: 'flushed_committed_mrs',
which gets set when the transactional MRS is flushed to disk. If true,
the transaction's write ops do not need to be replayed, since the
transaction does not have an active MRS. Otherwise, the transaction's
write ops do need to replayed, even if the transaction is persisted in
metadata as being committed.
Additionally, a transaction ID is added to the MutatedStorePB message
stored in the WAL, for the sake of replaying ops that land in committed,
transactional MRSs. Upon replay of such mutations, if the transactional
MRS being mutated to is not active (i.e. it has been flushed), the
mutation op can be ignored.
When starting up, we start transactional MRSs for all transactions that
do not have 'flushed_committed_mrs' set, which indicates that the
transaction's MRS was not flushed before restarting.
NOTE: the locking story here is not complete -- multiple transactions
can write to the same rows without consequence, which violates our row
uniqueness constraints. This will be addressed in future patches.
Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/txn_participant-itest.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/metadata.proto
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica-test-base.cc
M src/kudu/tablet/txn_metadata.h
M src/kudu/tablet/txn_participant-test.cc
M src/kudu/tablet/txn_participant.cc
M src/kudu/tablet/txn_participant.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
22 files changed, 1,307 insertions(+), 233 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/16515/2
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
Patch Set 4:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@39
PS3, Line 39: to
> to be
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/integration-tests/txn_participant-itest.cc
File src/kudu/integration-tests/txn_participant-itest.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/integration-tests/txn_participant-itest.cc@515
PS3, Line 515: write_threads.emplace_back([&, r, cur_row] {
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Ack
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h@388
PS3, Line 388: txn_id_ ? st
> nit: is this really necessary to construct this extra string?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc@715
PS3, Line 715:
> nit: spacing?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h
File src/kudu/tablet/ops/write_op.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@178
PS3, Line 178: // Acquires the lock on the given transaction, setting 'txn_' and
> nit: does it make sense to add a short doc comment for this method?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@285
PS3, Line 285:
> nit: add a short doc comment (this seems to be the only field without one
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet.cc@1439
PS3, Line 1439: CHECK(s == kOpen || s == kBootstrapping);
> warning: The left operand of '==' is a garbage value [clang-analyzer-core.U
Ack
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc@816
PS3, Line 816: void TabletMetadata::AddTxnMetadata(int64_t txn_id, unique_ptr<MinLogIndexAnchorer> log_anchor) {
> nit: an extra line?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc
File src/kudu/tablet/tablet_replica-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc@98
PS3, Line 98: if (resp.per_row_errors_size() > 0) {
: return StatusFromPB(resp.per_row_errors(0).error());
: }
> Good catch!
Ack
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h
File src/kudu/tablet/txn_metadata.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@43
PS3, Line 43: will ensure
> nit: 'will ensure' or just 'ensures'
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@44
PS3, Line 44: calls th
> nit: accessed
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@173
PS3, Line 173: if (resp.has_error()) {
: return StatusFromPB(resp.error().status());
: }
: return Status::OK();
: }
:
: // Writes an op to the WAL, rolls over onto a new WAL segment, and flushes
: // the MRS, leaving us with a new WAL segment that should be GC-able unless
: /
> It was a good call to separate this into a function.
Ack
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@740
PS3, Line 740: clock()->Now().value()));
: ASSERT_OK(IterateToStrings(&rows));
: ASSERT_EQ(0, rows.size());
> In addition, does it make sense to verify that no rows are visible if calli
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@844
PS3, Line 844:
> nit: could be constexpr ?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@851
PS3, Line 851: ASSERT_OK(CallPartici
> nit for here and below: would it make sense to add a scenario to try to wri
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@859
PS3, Line 859:
> Was it supposed to be ABORT_TXN instead?
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@869
PS3, Line 869: ASSERT_OK(Write(2, kAbortedTxnId));
> Does it make sense to be explicit in this test about the INSERT_IGNORE oper
I'm not sure what this is referring to. Perhaps the implicit INSERT op in Write() with no op argument? If so, I just removed it, since it isn't required to test unsupported ops.
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@871
PS3, Line 871: 2
> Does it make sense also to verify the behavior if using a not-present key f
I just removed the initial write, since it was not necessary.
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@901
PS3, Line 901: ASSERT_OK(CallParticipantO
> In addition, does it make sure that we are about to see the values from the
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@917
PS3, Line 917:
> Does it make sense to add a hybrid case as well, where a non-transactional
Done
I'm not keen on interleaving updated to the same row, since such interleaving should be disallowed by locking, which hasn't yet been implemented.
Done
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@1263
PS3, Line 1263: ()->metrics()->mrs_lookups->valu
> Does it make sense to add a hybrid case where in addition to disjoint trans
I'm holding off on all testing of concurrent mutations of the same rows, since such access patterns should be prevented via locking.
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc
File src/kudu/tablet/txn_participant.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc@60
PS3, Line 60: *txn_lock = std::m
> nit: is there any particular reason to prefer swap() to std::move() notatio
Done
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 18 Nov 2020 07:12:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
Patch Set 3:
(8 comments)
I did a quick first look, planning to take another look tomorrow. Just some nits and one high-level question so far.
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@13
PS3, Line 13: be scanned by any scanner
Does it mean that those would not be scanned even if using READ_YOUR_WRITES scanner mode?
I guess it's OK for first implementation, but do you think it would make sense to allow READ_YOUR_WRITES scanners to access uncommitted transaction data?
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@36
PS3, Line 36: when the transactional MRS is flushed to disk
I'm curious what happens if a kudu-tserver process crashes after flushing committing MRS, but before persisting the flag. Will kudu-tserver be able to restart, bootstart and work consistently after that?
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@39
PS3, Line 39: to
to be
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.h@388
PS3, Line 388: std::string(
nit: is this really necessary to construct this extra string?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/memrowset.cc@715
PS3, Line 715:
nit: spacing?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h
File src/kudu/tablet/ops/write_op.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@178
PS3, Line 178: Status AcquireTxnLockCheckOpen(scoped_refptr<Txn> txn);
nit: does it make sense to add a short doc comment for this method?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/ops/write_op.h@285
PS3, Line 285: scoped_refptr<TxnRowSets> txn_rowsets_;
nit: add a short doc comment (this seems to be the only field without one in this class)
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc
File src/kudu/tablet/txn_participant.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant.cc@60
PS3, Line 60: txn_lock->swap(l);
nit: is there any particular reason to prefer swap() to std::move() notation here? If not, maybe it's better to switch to std::move() to be in-line with the implementation on AcquireWriteLock() ?
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 13 Nov 2020 04:26:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@13
PS3, Line 13: be scanned by any scanner
> Does it mean that those would not be scanned even if using READ_YOUR_WRITES
That's right. RYW in the context of scanners is feasible, but I don't think it should be the default -- I think users should opt into scanning uncommitted data by providing a transaction ID when scanning.
http://gerrit.cloudera.org:8080/#/c/16515/3//COMMIT_MSG@36
PS3, Line 36: when the transactional MRS is flushed to disk
> I'm curious what happens if a kudu-tserver process crashes after flushing c
The flag is persisted at the time of flush, so if the flag didn't make it into the metadata, neither did that DRS, so we'd be left with an MRS.
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 13 Nov 2020 08:35:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
Patch Set 3:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_metadata.cc@816
PS3, Line 816:
nit: an extra line?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc
File src/kudu/tablet/tablet_replica-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/tablet_replica-test-base.cc@98
PS3, Line 98: if (resp.per_row_errors_size() > 0) {
: return StatusFromPB(resp.per_row_errors(0).error());
: }
Good catch!
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h
File src/kudu/tablet/txn_metadata.h:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@43
PS3, Line 43: will ensures
nit: 'will ensure' or just 'ensures'
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_metadata.h@44
PS3, Line 44: accesses
nit: accessed
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@173
PS3, Line 173: Status CallParticipantOpCheckResp(int64_t txn_id, ParticipantOpPB::ParticipantOpType op_type,
: int64_t ts_val) {
: ParticipantResponsePB resp;
: RETURN_NOT_OK(CallParticipantOp(tablet_replica_.get(), txn_id, op_type, ts_val, &resp));
: if (resp.has_error()) {
: return StatusFromPB(resp.error().status());
: }
: return Status::OK();
: }
It was a good call to separate this into a function.
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@740
PS3, Line 740: // Once we committed the transaction, we should see the rows.
: ASSERT_OK(CallParticipantOpCheckResp(kTxnId, ParticipantOpPB::FINALIZE_COMMIT,
: clock()->Now().value()));
In addition, does it make sense to verify that no rows are visible if calling ABORT_TXN after BEGIN_COMMIT?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@844
PS3, Line 844: kAbortedTxnId
nit: could be constexpr ?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@851
PS3, Line 851: s = Write(1, kTxnId);
nit for here and below: would it make sense to add a scenario to try to write already existing key (i.e. '0') to be explicit about the exact type of error we are about to receive in that case: Status::AlreadyPresent() or Status::InvalidArgument() ?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@859
PS3, Line 859: BEGIN_COMMIT
Was it supposed to be ABORT_TXN instead?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@869
PS3, Line 869: Status s = Write(0, kTxnId, RowOperationsPB::UPSERT);
Does it make sense to be explicit in this test about the INSERT_IGNORE operation?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@871
PS3, Line 871: 0
Does it make sense also to verify the behavior if using a not-present key for all those operations?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@901
PS3, Line 901: ASSERT_EQ(2, rows.size());
In addition, does it make sure that we are about to see the values from the expected transaction to appear? Maybe, in this regard it's enough to use different number of inserted rows for different transactions?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@917
PS3, Line 917: TestDontReadAbortedInserts
Does it make sense to add a hybrid case as well, where a non-transactional UPSERT happens on a row involved in a rolled-back transaction after the transaction rolled back?
Also, is it worth to arrange the sequence of transactional updates and non-transactional UPSERT such a way that the UPSERT start after the transaction on the same row begins, but before the transaction aborted?
What about sprinkling non-transactional UPDATE_IGNORE before and after aborted transactions and making sure nothing is updated?
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@1263
PS3, Line 1263: TestConcurrentDisjointInsertsTxn
Does it make sense to add a hybrid case where in addition to disjoint transactional inserts there concurrent non-transactional UPDATE_IGNORE on the same rows? Or that would make sense only after addressing the note in the commit description about addressing multiple updates into the same rowset?
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 14 Nov 2020 00:12:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add txn memrowsets to tablet
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16515 )
Change subject: KUDU-2612: add txn memrowsets to tablet
......................................................................
Patch Set 4: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@869
PS3, Line 869: ASSERT_OK(Write(2, kAbortedTxnId));
> I'm not sure what this is referring to. Perhaps the implicit INSERT op in W
Ah, I meant to add a scenario with INSERT_IGNORE for existing row, just to make sure it succeeds. But as I see the TestInsertIgnoreInTransactionMRS scenario contains that sequence, so all is well in this regard :)
http://gerrit.cloudera.org:8080/#/c/16515/3/src/kudu/tablet/txn_participant-test.cc@1263
PS3, Line 1263: ()->metrics()->mrs_lookups->valu
> I'm holding off on all testing of concurrent mutations of the same rows, si
I see. Thank you for the explanation.
http://gerrit.cloudera.org:8080/#/c/16515/4/src/kudu/util/locks.h
File src/kudu/util/locks.h:
http://gerrit.cloudera.org:8080/#/c/16515/4/src/kudu/util/locks.h@281
PS4, Line 281: shared_lock& operator=(shared_lock&& other) noexcept {
: swap(other);
: return *this;
Ah, cool -- I guess this was missing to start using std::move() for shared_lock
--
To view, visit http://gerrit.cloudera.org:8080/16515
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dea4031c0acb875993352d452dc4c233e35a502
Gerrit-Change-Number: 16515
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Nov 2020 04:17:28 +0000
Gerrit-HasComments: Yes