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/06/07 00:42:52 UTC
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16043
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet, which
reads from an underlying TabletReplica.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity in a later patch.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/status_tablet.cc
A src/kudu/transactions/status_tablet.h
A src/kudu/transactions/transactions.proto
7 files changed, 541 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/1
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#3).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet, which
reads from an underlying TabletReplica.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity in a later patch.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/status_tablet-test.cc
A src/kudu/transactions/status_tablet.cc
A src/kudu/transactions/status_tablet.h
A src/kudu/transactions/transactions.proto
8 files changed, 759 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/3
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#11).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
Partitioning isn't addressed in this patch, but I'm expecting later
patches to allow for the creation of partitioned transaction status
tables, and having the individual tablet replicas of that table be
accessed via this TxnStatusTablet API.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create or define partitions for
TxnStatusTablets on tablet servers.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 871 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/11
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 5:
(18 comments)
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h
File src/kudu/transactions/status_tablet.h:
PS4:
> To be more in-line with the name of the class, maybe rename these files int
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@19
PS4, Line 19:
> nit: use <cstdint> instead?
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@31
PS4, Line 31:
> nit: drop this extra line?
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@41
PS4, Line 41:
> Is this for the typedef or for the class TransactionVisitor? I guess it's
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@45
PS4, Line 45:
> Do you think it's worth describing the semantics of various return statuses
Actually this will never fail. Updated the type.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@58
PS4, Line 58:
> Would the value for this column be NULL for PARTICIPANT records? If so, ma
Good point -- I'd originally wanted to keep the 'metadata' record limited to the state that we expect to change per entry. That way any rewritten data can be limited to the state that has actually changed. That said, it is a bit of a premature optimization. Probably better to keep it simple for now.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@71
PS4, Line 71:
> I'm curious what 'physically thread-safe' means. Does it imply extra threa
Yeah, I suppose I just meant "thread-safe", though the primary caller of this (i.e. the TxnStatusManager) will need to enforce consistency on its end.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@90
PS4, Line 90:
> What is the relation between the schema of the tablet replica coming in her
I was trying to be a bit conservative w.rt. the schema, in case in the future we decided to change it, at least we could still operate the table. OTOH, I suppose it's straightforward enough to simply crash or fail the tablet in case we change schemas, and require some tooling to migrate this table.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@91
PS4, Line 91:
> nit: is it worth adding DCHECK(tablet_replica_) in the constructor?
Done
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc
File src/kudu/transactions/status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@193
PS2, Line 193:
> std::make_pair shouldn't be needed with emplace_back
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc
File src/kudu/transactions/status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@20
PS4, Line 20:
> nit: use <cstddef> ?
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@72
PS4, Line 72:
> Is the reference essential here? Why not simply 'const string' or even 'co
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@126
PS4, Line 126:
> Is it possible to do convert the known schema to PB just once and use it fo
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@139
PS4, Line 139:
:
> Why is it required? Does the transaction table store other than transactio
This is defensive, considering we've added entry types to the system catalog table. I'll add a comment.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@222
PS4, Line 222:
:
:
:
:
:
:
> Why not to make construct this once (e.g., use a static) and then return a
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@293
PS4, Line 293:
> Could this be constructed even without the underlying tablet replica, like
I was at first trying to be mindful of the possibility of schema changes, but I think that's a bit of a premature optimization.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@319
PS4, Line 319:
> Why Status::Corruption? Would Status::Incomplete() be a better option here
Done
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto
File src/kudu/transactions/transactions.proto:
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto@28
PS4, Line 28: -bones
> Where do we store the rest of metadata pertained to a transaction? Like 's
I left extraneous fields out and went with a simple message for now; we'll add those fields as they become necessary to implement transactions further.
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 13 Jun 2020 05:06:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 7: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc
File src/kudu/transactions/txn_status_tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@156
PS7, Line 156:
nit: ... and have the latest values ... ?
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h
File src/kudu/transactions/txn_status_tablet.h:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@72
PS7, Line 72: of
nit: drop?
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@76
PS7, Line 76: no
nit: not ?
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 01:30:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 2:
(5 comments)
Just took a high-level look. Some comments below.
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h
File src/kudu/transactions/status_tablet.h:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@45
PS2, Line 45: virtual Status VisitTransactionEntries(int64_t txn_id, std::string user,
Could you add a comment about this function?
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@63
PS2, Line 63: it's
: // easier to get the lowest value than it is to get the highest value
I don't understand why getting lowest value is easier than getting highest value.
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@96
PS2, Line 96: // Uses the given visitor to iterate over the entries in the rows of the
: // underlying tablet replica.
I'll take a look below but I don't understand the purpose of this function.
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc
File src/kudu/transactions/status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@78
PS2, Line 78: oid ExtractKeys
Any reason to keep these as standalone functions v/s making them private static functions of the TxnStatusTablet class?
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@193
PS2, Line 193: std::make_pair(
std::make_pair shouldn't be needed with emplace_back
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Jun 2020 22:12:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
Partitioning isn't addressed in this patch, but I'm expecting later
patches to allow for the creation of partitioned transaction status
tables, and having the individual tablet replicas of that table be
accessed via this TxnStatusTablet API.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create or define partitions for
TxnStatusTablets on tablet servers.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Reviewed-on: http://gerrit.cloudera.org:8080/16043
Reviewed-by: Attila Bukor <ab...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 871 insertions(+), 1 deletion(-)
Approvals:
Attila Bukor: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Andrew Wong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 12
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet-test.cc
File src/kudu/transactions/status_tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet-test.cc@80
PS2, Line 80: return { txn_id, std::move(user), std::move(txn_pb), std::move(prt_pbs) };
> warning: parameter 'user' is passed by value and only copied once; consider
Done
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h
File src/kudu/transactions/status_tablet.h:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@45
PS2, Line 45: // Signal to the visitor that a transaction exists with the given transaction
> Could you add a comment about this function?
Done
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@63
PS2, Line 63: a
: // transaction entry, participant entry, etc.
> I don't understand why getting lowest value is easier than getting highest
I'll remove the negative for now since I don't think it's useful to mention it yet, but the gist is that Kudu has the ability to perform an ordered scan, returning results lowest to highest. If this field is negative, finding the lowest value is as simple as performing an ordered scan with a limit on results size of 1.
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@93
PS2, Line 93: // contents of the tablet into a more usable memory representation.
> warning: single-argument constructors must be marked explicit to avoid unin
Done
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@96
PS2, Line 96: // Writes to the underlying storage. Returns an error if there was an error
: // writing the new entry.
> I'll take a look below but I don't understand the purpose of this function.
Updated the comment; I suppose it's clearer in the context of p2 of the patch series.
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc
File src/kudu/transactions/status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@78
PS2, Line 78: oid ExtractKeys
> Any reason to keep these as standalone functions v/s making them private st
I don't expect them to be used outside this compilation unit, so I don't see a reason to tie them to the TxnStatusTablet class.
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 09 Jun 2020 23:19:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#4).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet, which
reads from an underlying TabletReplica.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity in a later patch.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/status_tablet-test.cc
A src/kudu/transactions/status_tablet.cc
A src/kudu/transactions/status_tablet.h
A src/kudu/transactions/transactions.proto
8 files changed, 761 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/4
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Jun 2020 15:41:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Jun 2020 19:14:11 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto
File src/kudu/transactions/transactions.proto:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto@28
PS7, Line 28: TODO(awong): this is a bare-bones implementation so far. We'll certai
nit: indent or add a new line after the todo, it looks as if the description would be a continuation of the todo
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc
File src/kudu/transactions/txn_status_tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@258
PS7, Line 258: RET_IF_NOT_OK(status_tablet_->VisitTransactions(&visitor));
why are we returning instead of asserting? is this also failing the test?
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc
File src/kudu/transactions/txn_status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@81
PS7, Line 81: static KuduOnceLambda col_idx_initializer;
What's the benefit of using KuduOnceLambda over initializing these in a constructor? Also, aren't the column ids fixed if the schema is hardcoded? Seems like unnecessary function calls, but I could be missing something.
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@251
PS7, Line 251: if (PREDICT_FALSE(!prev_txn_id || *prev_txn_id != txn_id)) {
is the scan result guaranteed to be ordered (transaction, participant)? What if we add other entry types?
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 15:00:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#8).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
Partitioning isn't addressed in this patch, but I'm expecting later
patches to allow for the creation of partitioned transaction status
tables, and having the individual tablet replicas of that table be
accessed via this TxnStatusTablet API.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create or define partitions for
TxnStatusTablets on tablet servers.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 867 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/8
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
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/16043
to look at the new patch set (#2).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet, which
reads from an underlying TabletReplica.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity in a later patch.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/status_tablet-test.cc
A src/kudu/transactions/status_tablet.cc
A src/kudu/transactions/status_tablet.h
A src/kudu/transactions/transactions.proto
8 files changed, 749 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/2
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 9: Verified+1
Seems ASAN dist-test run timed out for some reason:
16:52:38 Fetching previously submitted C++ dist-test results...
16:52:38 ------------------------------------------------------------
16:52:39 INFO:dist_test.client:Watch your results at http://dist-test.cloudera.org/job?job_id=jenkins-slave.1592945318.22506
16:52:39 0.6s 0/432 tests complete
17:42:39 Build timed out (after 50 minutes). Marking the build as failed.
17:42:39 build-support/jenkins/build-and-test.sh: line 769: 4243 Terminated $DIST_TEST_HOME/bin/client watch $C_DIST_TEST_ID
though the tests all passed
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 21:59:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#9).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
Partitioning isn't addressed in this patch, but I'm expecting later
patches to allow for the creation of partitioned transaction status
tables, and having the individual tablet replicas of that table be
accessed via this TxnStatusTablet API.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create or define partitions for
TxnStatusTablets on tablet servers.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 867 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/9
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 23:13:32 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#5).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create a TxnStatusTablet on a tablet
server -- that will also come in later patches.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 865 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/5
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#6).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create a TxnStatusTablet on a tablet
server -- that will also come in later patches.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 865 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/6
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc
File src/kudu/transactions/txn_status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@81
PS7, Line 81: static KuduOnceLambda col_idx_initializer;
> Makes sense, thanks for the clarification. Do you think it's worth calling
Sure. I opted not to originally in order to completely decouple these calls from instantiations of TxnStatusTablet. But these getters are all in an anonymous namespace anyway, so I suppose restricting usage to after a TxnStatusTablet has been constructed wouldn't hurt.
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 24 Jun 2020 01:06:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 4:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/16043/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16043/4//COMMIT_MSG@10
PS4, Line 10: which
nit: I'm a bit confused here; could you clarify what reads what from where?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h
File src/kudu/transactions/status_tablet.h:
PS4:
To be more in-line with the name of the class, maybe rename these files into txn_status_tablet.{h,cc} ?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@19
PS4, Line 19: stdint.h
nit: use <cstdint> instead?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@31
PS4, Line 31:
nit: drop this extra line?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@41
PS4, Line 41: // Encapsulates the reading of state of the transaction status tablet.
Is this for the typedef or for the class TransactionVisitor? I guess it's for the latter, so maybe move this comment one line below.
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@45
PS4, Line 45: Signal
Do you think it's worth describing the semantics of various return statuses?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@58
PS4, Line 58: user STRING
Would the value for this column be NULL for PARTICIPANT records? If so, maybe the 'user' column should not be here, but instead be in the metadata?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@71
PS4, Line 71: physically thread-safe
I'm curious what 'physically thread-safe' means. Does it imply extra thread-safety guarantees compared with just 'thread-safe': could you add a bit more details here, please?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@90
PS4, Line 90: tablet_replica
What is the relation between the schema of the tablet replica coming in here as a parameter and the schema statically generated in TxnStatusTablet::GetSchema()? Should the schemas be identical? If so, maybe add a CHECK/DCHECK in the constructor or add some other way to preserve the consistency?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.h@91
PS4, Line 91: : tablet_replica_(tablet_replica) {}
nit: is it worth adding DCHECK(tablet_replica_) in the constructor?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc
File src/kudu/transactions/status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@20
PS4, Line 20: stddef.h
nit: use <cstddef> ?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@72
PS4, Line 72: &
Is the reference essential here? Why not simply 'const string' or even 'costexpr const char* const'?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@78
PS4, Line 78: const Schema& schema
If the schema of the tablet is known beforehand, why to pass it around and search for column indices every time?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@126
PS4, Line 126: SchemaToPB(schema, req.mutable_schema()
Is it possible to do convert the known schema to PB just once and use it for building write request PB messages?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@139
PS4, Line 139: record_types.push_back(TRANSACTION);
: record_types.push_back(PARTICIPANT);
Why is it required? Does the transaction table store other than transaction records?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@222
PS4, Line 222: SchemaBuilder builder;
: CHECK_OK(builder.AddKeyColumn(kTxnIdColName, INT64));
: CHECK_OK(builder.AddKeyColumn(kEntryTypeColName, INT8));
: CHECK_OK(builder.AddKeyColumn(kIdentifierColName, STRING));
: CHECK_OK(builder.AddNullableColumn(kUserColName, STRING));
: CHECK_OK(builder.AddColumn(kMetadataColName, STRING));
: return builder.Build();
Why not to make construct this once (e.g., use a static) and then return a constant reference to that?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@293
PS4, Line 293: tablet_replica_->tablet()->schema()->CopyWithoutColumnIds();
Could this be constructed even without the underlying tablet replica, like in TxnStatusTablet::GetSchema()? Or it's intentional that this is taken directly from the underlying tablet? If the latter is true, then maybe rename this method to reflect the fact somehow (like GetActualTabletSchema(), etc.)?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/status_tablet.cc@319
PS4, Line 319: Corruption
Why Status::Corruption? Would Status::Incomplete() be a better option here?
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto
File src/kudu/transactions/transactions.proto:
http://gerrit.cloudera.org:8080/#/c/16043/4/src/kudu/transactions/transactions.proto@28
PS4, Line 28: status
Where do we store the rest of metadata pertained to a transaction? Like 'start' timestamp, etc.?
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
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: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 11 Jun 2020 17:54:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#7).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create a TxnStatusTablet on a tablet
server -- that will also come in later patches.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 865 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/7
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16043
to look at the new patch set (#10).
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
KUDU-2612 p1: add initial transaction status storage
This adds a system tablet storage API for storing the status of
transactions, in the form of the newly added TxnStatusTablet which is a
wrapper around a TabletReplica with a schema tailored for storing
transaction metadata.
The abstraction is comparable to the SysCatalogTable abstraction used by
the master to store metadata about the Kudu catalog, but rather than
storing metadata about tables and tablets, the TxnStatusTablet stores
metadata about transactions and transaction participants.
Partitioning isn't addressed in this patch, but I'm expecting later
patches to allow for the creation of partitioned transaction status
tables, and having the individual tablet replicas of that table be
accessed via this TxnStatusTablet API.
This patch only introduces the schema, basic write calls, and scan calls
to be used by a transaction management entity to be added in a later
patch. There is currently no way to create or define partitions for
TxnStatusTablets on tablet servers.
Change-Id: I94ddbd37c65932120835d6e138307f819935173c
---
M CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
A src/kudu/transactions/CMakeLists.txt
A src/kudu/transactions/transactions.proto
A src/kudu/transactions/txn_status_tablet-test.cc
A src/kudu/transactions/txn_status_tablet.cc
A src/kudu/transactions/txn_status_tablet.h
8 files changed, 873 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/16043/10
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 9: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc
File src/kudu/transactions/txn_status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@81
PS7, Line 81: static KuduOnceLambda col_idx_initializer;
> > What's the benefit of using KuduOnceLambda over initializing these in a c
Makes sense, thanks for the clarification. Do you think it's worth calling the KuduOnceLambda in the constructor instead to save the extra function calls? It might be premature optimization though, and also the compiler would probably optimize it out anyway (haven't checked).
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@251
PS7, Line 251: if (PREDICT_FALSE(!prev_txn_id || *prev_txn_id != txn_id)) {
> Yeah, the scan spec is ordered and only includes TRANSACTION and PARTICIPAN
ah right, for some reason I thought that InList could include other types in the future.
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 22:00:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 8:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto
File src/kudu/transactions/transactions.proto:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/transactions.proto@28
PS7, Line 28: TODO(awong): this is a bare-bones implementation so far. We'll certai
> nit: indent or add a new line after the todo, it looks as if the descriptio
Done
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc
File src/kudu/transactions/txn_status_tablet-test.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@156
PS7, Line 156:
> nit: ... and have the latest values ... ?
Done
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet-test.cc@258
PS7, Line 258: SimpleTransactionsVisitor visitor;
> why are we returning instead of asserting? is this also failing the test?
This is just to make failure a bit more graceful -- rather than failing in a thread, the macro is collecting the Statuses and checking for errors down below, which should make it a bit nicer, especially if multiple threads fail.
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h
File src/kudu/transactions/txn_status_tablet.h:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@72
PS7, Line 72:
> nit: drop?
Done
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.h@76
PS7, Line 76: no
> nit: not ?
Done
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc
File src/kudu/transactions/txn_status_tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@81
PS7, Line 81: static KuduOnceLambda col_idx_initializer;
> What's the benefit of using KuduOnceLambda over initializing these in a constructor?
There may be multiple transaction status tablets in a given tablet server, so we may end up calling the constructor multiple times, creating multiple of the same schema in the process. Putting this in a KuduOnceLambda ensures we only do that work once.
> Also, aren't the column ids fixed if the schema is hardcoded?
Column IDs and indexes are somewhat fixed, but this is a bit more defensive -- the API we've defined for schemas revolves around calling these methods and I'd rather rely on the API rather than some internal details, even if we don't expect them to change.
Also, we're using column indexes here instead of column IDs because lookups by column indexes are trivial.
> Seems like unnecessary function calls, but I could be missing something.
Yeah, but we're only doing them once, and IMO it makes the call-sites more legible.
http://gerrit.cloudera.org:8080/#/c/16043/7/src/kudu/transactions/txn_status_tablet.cc@251
PS7, Line 251: if (PREDICT_FALSE(!prev_txn_id || *prev_txn_id != txn_id)) {
> is the scan result guaranteed to be ordered (transaction, participant)? Wha
Yeah, the scan spec is ordered and only includes TRANSACTION and PARTICIPANT records. See the initialization at L201.
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 23 Jun 2020 19:51:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p1: add initial transaction status storage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage
......................................................................
Patch Set 11: Verified+1
Unrelated flaky test: TabletServerDiskError/TabletServerDiskErrorITest.TestFailDuringScanWorkload/0
--
To view, visit http://gerrit.cloudera.org:8080/16043
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c
Gerrit-Change-Number: 16043
Gerrit-PatchSet: 11
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 25 Jun 2020 19:56:10 +0000
Gerrit-HasComments: No