[kudu-CR] KUDU-2612: initial implementation of TxnManager
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16527 to look at the new patch set (#7). Change subject: KUDU-2612: initial implementation of TxnManager .. KUDU-2612: initial implementation of TxnManager This is a first implementation of the TxnManager. The TxnManager class encapsulates the logic used by the TxnManagerService while serving RPC requests (see txn_manager.proto for the protobuf interface). The most essential piece of the logic to be implemented by this class is the assignment of an identifier for a new transaction and initialization of the transaction status table, along with creating of its new partitions, when needed. All other methods simply do proxying of corresponding requests to the underlying instance of TxnSystemClient. This changelist also contains test scenarios to cover the newly introduced functionality. The implementation of TxnManager::BeginTransaction() is moved into a follow-up changelist by request for simplify the process of reviewing these changes. TxnManager::KeepTransactionAlive() will be implemented as soon as the corresponding functionality is ready in the TxnStatusManager. Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c --- M src/kudu/master/CMakeLists.txt M src/kudu/master/master.cc M src/kudu/master/master.h A src/kudu/master/txn_manager-test.cc A src/kudu/master/txn_manager.cc A src/kudu/master/txn_manager.h A src/kudu/master/txn_manager.proto A src/kudu/master/txn_manager_service.cc A src/kudu/master/txn_manager_service.h M src/kudu/transactions/txn_status_manager.h M src/kudu/transactions/txn_system_client.cc 11 files changed, 1,203 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16527/7 -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: initial implementation of TxnManager
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16527 to look at the new patch set (#6). Change subject: KUDU-2612: initial implementation of TxnManager .. KUDU-2612: initial implementation of TxnManager This is a first implementation of the TxnManager. The TxnManager class encapsulates the logic used by the TxnManagerService while serving RPC requests (see txn_manager.proto for the protobuf interface). The most essential piece of the logic to be implemented by this class is the assignment of an identifier for a new transaction and initialization of the transaction status table, along with creating of its new partitions, when needed. All other methods simply do proxying of corresponding requests to the underlying instance of TxnSystemClient. This changelist also contains test scenarios to cover the newly introduced functionality. The implementation of TxnManager::BeginTransaction() is moved into a follow-up changelist by request for simplify the process of reviewing these changes. TxnManager::KeepTransactionAlive() will be implemented as soon as the corresponding functionality is ready in the TxnStatusManager. Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c --- M src/kudu/master/CMakeLists.txt M src/kudu/master/master.cc M src/kudu/master/master.h A src/kudu/master/txn_manager-test.cc A src/kudu/master/txn_manager.cc A src/kudu/master/txn_manager.h A src/kudu/master/txn_manager.proto A src/kudu/master/txn_manager_service.cc A src/kudu/master/txn_manager_service.h M src/kudu/transactions/txn_status_manager.h M src/kudu/transactions/txn_system_client.cc 11 files changed, 1,201 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/16527/6 -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: initial implementation of TxnManager
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16527 ) Change subject: KUDU-2612: initial implementation of TxnManager .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@97 PS3, Line 97: true > Yea, in general I think it's a good practice to have feature flags but keep OK, done -- this flag is set to 'false' by default. http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@318 PS3, Line 318: retry_interval > Seems PS5 missed this Whoops, indeed. But I remember I changed this. Anyways, now this should be fixed. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc@103 PS5, Line 103: Whether to initialize TxnManager upon arrival of first request. > Wondering how to decide if TxnManager should be initialized upon arrival of I guess for real world scenarios in clusters which use transactions we should prefer non-lazy initialization. The lazy initialization mode helps to get away with too many (60+) broken tests otherwise. At some point we should introduce proper tablet filtering or explicitly set --txn_manager_lazily_initialized=true in those broken tests, and change this flag to be 'false' by default. I added corresponding comment. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc@319 PS5, Line 319: KLOG_EVERY_N_SECS(WARNING, 60) << Substitute( : "$0: unable to init TxnManager, will retry in $1", : s.ToString(), retry_interval.ToString()); : SleepFor(retry_interval); > Why do we want to retry the initialization, but not as what catalog manager TxnManager cannot complete initialization without tablet servers which host the txn status table's tablets. However, master can be started when no tablet servers are running. If the only mode that TxnManager would be running with were lazy initialization, not retrying this task might be an option, but I guess in real world scenarios we should prefer non-lazy initialization, hence the retry logic. Also, current behavior makes it much easier to handle waiting on the txn_manager_init_status_ promise. I added a comment on the rationale behind this task's behavior. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.h File src/kudu/master/txn_manager.h: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.h@102 PS5, Line 102: static constexpr int64_t kTxnTableRangeStep = 1024; > Can you add a comment to explain why 1024 is chosen? There is nothing specific about this, it's just a random number so far. I moved this to be a flag and added description for the flag. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc File src/kudu/master/txn_manager.cc: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc@49 PS5, Line 49: If 's' is not OK and 'resp' has no application specific error set > Do you know in which cases this will happen? It happens in case of RPC-level errors. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc@127 PS5, Line 127: // This method isn't supposed to be called concurrently, so there isn't any : // protection against concurrent calls. > Should this be moved to the method declaration of the header. The header has similar comment as well. I put this comment here as well for better readability. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto File src/kudu/master/txn_manager.proto: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto@94 PS5, Line 94: Whether > nit: "What the transaction state was.." Replaced with: The transaction state at the time of processing the request http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto@94 PS5, Line 94: was > nit: was OK? I replaced the whole sentence with 'The transaction state at the time of processing the request' -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 14 Oct 2020 04:11:50 + Gerrit-HasComments: Yes
[kudu-CR] [partitioning] KUDU-2671: Support for range specific HashSchemas.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 ) Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@389 PS1, Line 389: std::vector > nit: I wonder if it makes sense to typedef vector as some Oops, s/HashBucketSpec/HashBucketSchemas This would also be useful for hash_partition_schemas(). -- To view, visit http://gerrit.cloudera.org:8080/16596 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 Gerrit-Change-Number: 16596 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Wed, 14 Oct 2020 02:41:35 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. KUDU-3195: flush when any DMS in the tablet is older than the time threshold Currently each tablet will wait at least 2 minutes (controlled by --flush_threshold_secs) between flushing DMSs, even if there are several DMSs that are older than 2 minutes in a given tablet. This means that for tablets with several dozen rowsets and updates across the entire tablet, it could take hours to flush all the deltas. Rather than waiting for 2 minutes since the last flush time before considering time-based flushing, this patch tracks the creation time of every DMS and flushes as long as there is a DMS that is older than 2 minutes in the tablet. Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Reviewed-on: http://gerrit.cloudera.org:8080/16581 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tablet_server-test.cc 14 files changed, 234 insertions(+), 150 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [partitioning] KUDU-2671: Support for range specific HashSchemas.
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16596 )
Change subject: [partitioning] KUDU-2671: Support for range specific
HashSchemas.
..
Patch Set 1:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG@12
PS1, Line 12: Open to discussion on whether or
: not we should support this feature with split_rows.
nit: it'd be good to add some context as to what this would entail, what
user-facing effects this has, etc. Or outline the problem in the design doc so
it can be discussed further.
http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@909
PS1, Line 909: hashSchema1
nit: snake_case, same elsewhere
http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@912
PS1, Line 912:
hashSchema2.emplace_back(PartitionSchema::HashBucketSchema{{ColumnId(0)}, 2,
0});
:
hashSchema2.emplace_back(PartitionSchema::HashBucketSchema{{ColumnId(1)}, 3,
0});
nit: pretty sure you can initialize this via initializer list, e.g.
Vector hash_schema2 = {
{ { ColumnId(0) }, 2, 0 },
{ { ColumnId(1) }, 3, 0 }
};
http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@919
PS1, Line 919: 16
nit: could you add a comment explaining this value? May also be able to adjust
variable names to make this a bit clearer, e.g.
default_hash_buckets_with_3_by_2 = { HashBucketSchema(3), HashBucketSchema(2)
};
hash_buckets_with_4 = { HashBucketSchema(4) };
hash_buckets_with_2_by_3 = { HashBucketSchema(2), HashBucketSchema(3) };
At least right now, a reader trying to follow/verify this code would have read
between the lines, parse out exactly what ranges have what, and then do the
math.
http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition-test.cc@921
PS1, Line 921: EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <=
VALUES < (1, -2147483648, 3), "
: "HASH (a) PARTITION 0",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[0], range_hash_schema[0].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <=
VALUES < (1, -2147483648, 3), "
: "HASH (a) PARTITION 1",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[1], range_hash_schema[0].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <=
VALUES < (1, -2147483648, 3), "
: "HASH (a) PARTITION 2",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[2], range_hash_schema[0].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (0, -2147483648, 2) <=
VALUES < (1, -2147483648, 3), "
: "HASH (a) PARTITION 3",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[3], range_hash_schema[0].second,
schema));
:
: EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <=
VALUES < (2, 3, -2147483648), "
: "HASH (a, c) PARTITION 0, HASH (b) PARTITION 0",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[4], range_hash_schema[1].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <=
VALUES < (2, 3, -2147483648), "
: "HASH (a, c) PARTITION 0, HASH (b) PARTITION 1",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[5], range_hash_schema[1].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <=
VALUES < (2, 3, -2147483648), "
: "HASH (a, c) PARTITION 1, HASH (b) PARTITION 0",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[6], range_hash_schema[1].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <=
VALUES < (2, 3, -2147483648), "
: "HASH (a, c) PARTITION 1, HASH (b) PARTITION 1",
: partition_schema.HashSchemaPerPartitionDebugString
: (partitions[7], range_hash_schema[1].second,
schema));
: EXPECT_EQ("RANGE (a, b, c) PARTITION (1, 1, -2147483648) <=
VALUES < (2, 3, -2147483648), "
:
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 6: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc@2234 PS5, Line 2234: // To facilitate memory-based flushing when under memory pressure, we : // define a score that's part memory and part WAL retention bytes. : double score = dms_size_bytes * mem_weight + replay_size_bytes * (100 - mem_weight); > It is not reused anywhere right now, and I actually question its value. I'm SGTM -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 14 Oct 2020 01:46:26 + Gerrit-HasComments: Yes
[kudu-CR] [partitioning] KUDU-2671: Support for range specific HashSchemas.
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16596 Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. .. [partitioning] KUDU-2671: Support for range specific HashSchemas. This patch updates Partition::CreatePartitions() to support the ability to add different hash schemas per each range. If no hash schema per range is specified, the table wide hash schema is used. Currently, this only works if no split_rows are specified. Open to discussion on whether or not we should support this feature with split_rows. Currently, RangeHashSchema within partition.h maps each range's vector of hash schemas to an integer. Ideally, it would be mapped to the range's upper and lower bounds but if we support split_rows the bounds would not be finalized until within CreatePartitions(). Inspiration from Vlad: https://gerrit.cloudera.org/c/15758/ Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 --- M src/kudu/common/partition-test.cc M src/kudu/common/partition.cc M src/kudu/common/partition.h M src/kudu/common/partition_pruner-test.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet-harness.h M src/kudu/tserver/tablet_server-test.cc 9 files changed, 258 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/16596/1 -- To view, visit http://gerrit.cloudera.org:8080/16596 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 Gerrit-Change-Number: 16596 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
:
highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
> I don't think reversing the order make sense: stored_txn_id is updated by t
Ah right. SGTM.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 00:11:04 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
:
highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
> Maybe reverse the order so we don't have to compare the atomic if stored_tx
I don't think reversing the order make sense: stored_txn_id is updated by the
compare_exchange_strong() if it returns false, so it's essential to get its
value _after_ the call to compare_exchange_strong.
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 14 Oct 2020 00:02:54 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 6: (14 comments) http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG@9 PS5, Line 9: least 2 minutes > nit: maybe, it's worth pointing that it's controlled by the --flush_thresho Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@238 PS5, Line 238: a D > nit: drop Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239 PS5, Line 239: . Otherwise, returns false and doesn't upda > nit: maybe, it's worth mentioning what's the expectation on filling in the Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239 PS5, Line 239: ich it was create > nit: maybe, name it as GetDMSInfo()/GetDeltaMemStoreInfo() or alike, pointi Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@867 PS5, Line 867: // Check dms_exists_ first to avoid unnecessary contention on : // component_lock_. We ne > nit: do we expect high contention on component_lock_? If so, then maybe tr Good point. We do this in other methods, so we might as well do so here. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@873 PS5, Line 873: *size_bytes = dms_->EstimateSize(); : *creation_time = dms_->c > Is it really necessary to bother filling in these if DMS isn't there? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h@381 PS5, Line 381: return false; : } > nit: why to bother filling in these if returning 'false' anyways? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h@429 PS5, Line 429: virtual Status DebugDump(std::vector *lines = nullptr) override; > warning: use nullptr [modernize-use-nullptr] Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h@436 PS5, Line 436: return nullptr; > warning: use nullptr [modernize-use-nullptr] Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc@2234 PS5, Line 2234: // To facilitate memory-based flushing when under memory pressure, we : // define a score that's part memory and part WAL retention bytes. : double score = dms_size_bytes * mem_weight + replay_size_bytes * (100 - mem_weight); > Should it be moved into a method/function? I guess it might be re-used in It is not reused anywhere right now, and I actually question its value. I'm not sure the assessment on KUDU-2238 is complete, given they waited hours for a flush to happen. For now, I'm leaving it as is. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc File src/kudu/tablet/tablet_replica_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc@268 PS5, Line 268: now - earliest_dms_time).ToMillisec > What if FindBestDMSToFlush() didn't find any DMS? Does this stay semantica Negative MonoDeltas are valid, though I'll update this to be more conservative with the MonoTime::Max() case. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4335 PS5, Line 4335: KUDU-3195 > KUDU-3195 ? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4356 PS5, Line 4356: > nit: maybe, using base::NumCPUs() would look a bit more natural? Actually this is fairly unimportant; I'll just stick with a single MM thread. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4362 PS5, Line 4362: r > Just to make sure I understand: does this test create enough updates/deltas Yep, there are 100 rowsets, and therefore 100 DMSs to flush. Before, we would wait 1 second between scheduling each one, so with a single thread, that would take 100s. -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id:
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16581 to look at the new patch set (#6). Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. KUDU-3195: flush when any DMS in the tablet is older than the time threshold Currently each tablet will wait at least 2 minutes (controlled by --flush_threshold_secs) between flushing DMSs, even if there are several DMSs that are older than 2 minutes in a given tablet. This means that for tablets with several dozen rowsets and updates across the entire tablet, it could take hours to flush all the deltas. Rather than waiting for 2 minutes since the last flush time before considering time-based flushing, this patch tracks the creation time of every DMS and flushes as long as there is a DMS that is older than 2 minutes in the tablet. Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tablet_server-test.cc 14 files changed, 234 insertions(+), 150 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/16581/6 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612: add TxnManager::BeginTransaction()
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16586 )
Change subject: KUDU-2612: add TxnManager::BeginTransaction()
..
Patch Set 2: Code-Review+1
(6 comments)
Overall LGTM. Just some nits left.
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager-test.cc@426
PS1, Line 426:
> I'm not sure emplace() would fit here: that's a range insert. At least, I
Yep, you're right. emplace() doesn't take in iterators like this.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc
File src/kudu/master/txn_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@405
PS2, Line 405: static constexpr int64_t kNumTransactions = 1;
nit: Would it also make sense to check that we have added several new tablets
for the transaction status table, to more explicitly test the
AddNewRangePartition logic?
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager-test.cc@407
PS2, Line 407: txn_initiator(kNumTransactions, _ids);
nit: not that we expect concurrency issues here, but maybe also dedupe these
before comparing?
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/1/src/kudu/master/txn_manager.cc@119
PS1, Line 119: // with the highest 'highest_seen_txn_id' so far updating
'next_txn_id_'
: // until it succeeds or bail if there is another thread
that received a
: // greater 'highest_seen_txn_id' back from
TxnStatusManager.
: int64_t stored_txn_id = try_txn_id + 1;
: while (true) {
:
> Ah, I guess there was a swap of the 'try_txn_id' and 'highest_seen_txn_id',
Yep, this is a lot clearer now.
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc
File src/kudu/master/txn_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@123
PS2, Line 123: while (true) {
nit: should we DCHECK that highest_seen_txn_id >= 0 here too?
http://gerrit.cloudera.org:8080/#/c/16586/2/src/kudu/master/txn_manager.cc@124
PS2, Line 124: next_txn_id_.compare_exchange_strong(stored_txn_id,
:
highest_seen_txn_id + 1) ||
: stored_txn_id > highest_seen_txn_id) {
Maybe reverse the order so we don't have to compare the atomic if stored_txn_id
> highest_seen_txn_id?
--
To view, visit http://gerrit.cloudera.org:8080/16586
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I51c476d92bb5b147ffd03fd9f3163ab86d581496
Gerrit-Change-Number: 16586
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 23:02:37 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3202: [build] Add Spark 3 Support
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16582 ) Change subject: KUDU-3202: [build] Add Spark 3 Support .. KUDU-3202: [build] Add Spark 3 Support This patch adds Sparks 3 support to the build and builds using Spark 3 by default while maintiaining the ability to build Spark 2. One simple change to the backup job was required to maintain maximum backwards compatibility due to SPARK-31404. Additionally a few small changes were needed to fix SpotBugs issues. Follow on changes will adjust build scripts and publishing docs to publish both Spark 2 and 3 Jars. Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f Reviewed-on: http://gerrit.cloudera.org:8080/16582 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M java/config/spotbugs/excludeFilter.xml M java/gradle/dependencies.gradle M java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupUtils.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 6 files changed, 42 insertions(+), 8 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f Gerrit-Change-Number: 16582 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [Java] Upgrade dependencies
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16583 ) Change subject: [Java] Upgrade dependencies .. [Java] Upgrade dependencies Upgrades the Java dependencies and Gradle versions. Minor version upgrades: - jmh 1.25.2 -> 1.26 Maintenance version upgrades: - httpClient 4.5.12 -> 4.5.13 - jetty 9.4.31.v20200723 -> 9.4.32.v20200930 - junit 4.13 -> 4.13.1 - micrometer 1.5.4 -> 1.5.5 - mockito 3.5.10 -> 3.5.13 Gradle upgrades: - shadow 6.0.0 -> 6.1.0 Change-Id: If7febccddbf080520ebdf6b4b71cc1571df1927b Reviewed-on: http://gerrit.cloudera.org:8080/16583 Tested-by: Grant Henke Reviewed-by: Andrew Wong --- M java/buildSrc/build.gradle M java/gradle/dependencies.gradle 2 files changed, 7 insertions(+), 7 deletions(-) Approvals: Grant Henke: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If7febccddbf080520ebdf6b4b71cc1571df1927b Gerrit-Change-Number: 16583 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3202: [build] Add Spark 3 Support
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/16582 )
Change subject: KUDU-3202: [build] Add Spark 3 Support
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16582/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/16582/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@89
PS3, Line 89:
session.conf.set("spark.sql.legacy.parquet.datetimeRebaseModeInWrite", "LEGACY")
> Just curious, when we deprecate Spark 2, will we need to update this?
We can remove it when we drop all support for Spark 2, but also we don't need
to either.
--
To view, visit http://gerrit.cloudera.org:8080/16582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f
Gerrit-Change-Number: 16582
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 19:49:03 +
Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Avoid transitive log4j dependencies
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16592 ) Change subject: [subprocess] Avoid transitive log4j dependencies .. [subprocess] Avoid transitive log4j dependencies This patch excludes the log4j and slf4j dependencies from the Hadoop and Ranger dependencies to ensure we use only our explicitly defined versions. This patch also adds a test log4j2.properties file that was missing. Change-Id: I48db5ee55e0f8d9f38b0351b2b3f7509175f6320 Reviewed-on: http://gerrit.cloudera.org:8080/16592 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M java/kudu-subprocess/build.gradle A java/kudu-subprocess/src/test/resources/log4j2.properties 2 files changed, 39 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I48db5ee55e0f8d9f38b0351b2b3f7509175f6320 Gerrit-Change-Number: 16592 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3202: [build] Add Spark 3 Support
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16582 )
Change subject: KUDU-3202: [build] Add Spark 3 Support
..
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16582/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala:
http://gerrit.cloudera.org:8080/#/c/16582/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala@89
PS3, Line 89:
session.conf.set("spark.sql.legacy.parquet.datetimeRebaseModeInWrite", "LEGACY")
Just curious, when we deprecate Spark 2, will we need to update this?
--
To view, visit http://gerrit.cloudera.org:8080/16582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f
Gerrit-Change-Number: 16582
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 19:42:37 +
Gerrit-HasComments: Yes
[kudu-CR] [Java] Upgrade dependencies
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16583 ) Change subject: [Java] Upgrade dependencies .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7febccddbf080520ebdf6b4b71cc1571df1927b Gerrit-Change-Number: 16583 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 19:37:53 + Gerrit-HasComments: No
[kudu-CR] [subprocess] Avoid transitive log4j dependencies
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16592 ) Change subject: [subprocess] Avoid transitive log4j dependencies .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48db5ee55e0f8d9f38b0351b2b3f7509175f6320 Gerrit-Change-Number: 16592 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 19:36:55 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612: initial implementation of TxnManager
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16527 ) Change subject: KUDU-2612: initial implementation of TxnManager .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 19:26:12 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612: initial implementation of TxnManager
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16527 ) Change subject: KUDU-2612: initial implementation of TxnManager .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@97 PS3, Line 97: true > Right -- with lazily initialized TxnManager, this isn't an issue. We could Yea, in general I think it's a good practice to have feature flags but keep them off by default until the feature is done, since this consumes some memory and resources to initialize the service, etc. until then. http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@318 PS3, Line 318: retry_interval > Done Seems PS5 missed this http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto File src/kudu/master/txn_manager.proto: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto@94 PS5, Line 94: Whether nit: "What the transaction state was.." -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 19:26:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/16581 )
Change subject: KUDU-3195: flush when any DMS in the tablet is older than the
time threshold
..
Patch Set 5:
(12 comments)
Just did a quick look. Overall looks good, but I guess I'll need to take a
second look into the essential changes in tablet.cc
http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG@9
PS5, Line 9: least 2 minutes
nit: maybe, it's worth pointing that it's controlled by the
--flush_threshold_secs flag, i.e. it's not hard-coded?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h
File src/kudu/tablet/delta_tracker.h:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@238
PS5, Line 238: the
nit: drop
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239
PS5, Line 239: size_t* size_bytes, MonoTime* creation_time
nit: maybe, it's worth mentioning what's the expectation on filling in the
values of the output parameters?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239
PS5, Line 239: DeltaMemStoreInfo
nit: maybe, name it as GetDMSInfo()/GetDeltaMemStoreInfo() or alike, pointing
that the method retrieves info on a DMS.
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc
File src/kudu/tablet/delta_tracker.cc:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@867
PS5, Line 867: shared_lock lock(component_lock_);
: if (dms_exists_.Load()) {
nit: do we expect high contention on component_lock_? If so, then maybe try
first to check the atomic before taking a shared lock (yes, it's uglier, but it
might help with lock contention):
if (dms_exists_.Load()) {
shared_lock lock(component_lock_);
if (dms_exists_.Load()) {
...
}
}
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@873
PS5, Line 873: *size_bytes = 0;
: *creation_time = MonoTime();
Is it really necessary to bother filling in these if DMS isn't there?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h
File src/kudu/tablet/memrowset.h:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h@381
PS5, Line 381: *size_bytes = 0;
: *creation_time = MonoTime();
nit: why to bother filling in these if returning 'false' anyways?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc@2234
PS5, Line 2234: // To facilitate memory-based flushing when under memory
pressure, we
: // define a score that's part memory and part WAL retention
bytes.
: double score = dms_size_bytes * mem_weight +
replay_size_bytes * (100 - mem_weight);
Should it be moved into a method/function? I guess it might be re-used in
other places as well.
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc
File src/kudu/tablet/tablet_replica_mm_ops.cc:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc@268
PS5, Line 268: MonoTime::Now() - earliest_dms_time
What if FindBestDMSToFlush() didn't find any DMS? Does this stay semantically
valid?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4335
PS5, Line 4335: KUDU-3145
KUDU-3195 ?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4356
PS5, Line 4356: 10
nit: maybe, using base::NumCPUs() would look a bit more natural?
http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4362
PS5, Line 4362: 5
Just to make sure I understand: does this test create enough updates/deltas to
span for longer that 5 flush thresholds if there is a regression? Maybe, it's
worth adding a comment to provide more information on the expected speed of DMS
flushing given the parameters of this test scenario.
--
To view, visit http://gerrit.cloudera.org:8080/16581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4
Gerrit-Change-Number: 16581
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 13 Oct 2020 19:21:05 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612: initial implementation of TxnManager
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16527 ) Change subject: KUDU-2612: initial implementation of TxnManager .. Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc@103 PS5, Line 103: Whether to initialize TxnManager upon arrival of first request. Wondering how to decide if TxnManager should be initialized upon arrival of first request or the other? http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/master.cc@319 PS5, Line 319: KLOG_EVERY_N_SECS(WARNING, 60) << Substitute( : "$0: unable to init TxnManager, will retry in $1", : s.ToString(), retry_interval.ToString()); : SleepFor(retry_interval); Why do we want to retry the initialization, but not as what catalog manager is doing which don't retry? http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.h File src/kudu/master/txn_manager.h: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.h@102 PS5, Line 102: static constexpr int64_t kTxnTableRangeStep = 1024; Can you add a comment to explain why 1024 is chosen? http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc File src/kudu/master/txn_manager.cc: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc@49 PS5, Line 49: If 's' is not OK and 'resp' has no application specific error set Do you know in which cases this will happen? http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.cc@127 PS5, Line 127: // This method isn't supposed to be called concurrently, so there isn't any : // protection against concurrent calls. Should this be moved to the method declaration of the header. http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto File src/kudu/master/txn_manager.proto: http://gerrit.cloudera.org:8080/#/c/16527/5/src/kudu/master/txn_manager.proto@94 PS5, Line 94: was nit: was OK? -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 18:59:56 + Gerrit-HasComments: Yes
[kudu-CR] [subprocess] Avoid transitive log4j dependencies
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16592 Change subject: [subprocess] Avoid transitive log4j dependencies .. [subprocess] Avoid transitive log4j dependencies This patch excludes the log4j and slf4j dependencies from the Hadoop and Ranger dependencies to ensure we use only our explicitly defined versions. This patch also adds a test log4j2.properties file that was missing. Change-Id: I48db5ee55e0f8d9f38b0351b2b3f7509175f6320 --- M java/kudu-subprocess/build.gradle A java/kudu-subprocess/src/test/resources/log4j2.properties 2 files changed, 39 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/16592/1 -- To view, visit http://gerrit.cloudera.org:8080/16592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I48db5ee55e0f8d9f38b0351b2b3f7509175f6320 Gerrit-Change-Number: 16592 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] wip KUDU-3149: don't block op registration on MM mutex
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16580 ) Change subject: wip KUDU-3149: don't block op registration on MM mutex .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc@302 PS4, Line 302: emplace > It looks like emplace is valid C++11 Right, the 'emplace' method signature assumes just a single element to add into the container, but here is a range. Anyways, we will see whether it's compilable or not :) -- To view, visit http://gerrit.cloudera.org:8080/16580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a1b810f5b7ff6a22acc9b10b79ddffa8085c990 Gerrit-Change-Number: 16580 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 18:26:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 5: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 17:59:07 + Gerrit-HasComments: No
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/16581/4/src/kudu/tablet/tablet_replica_mm_ops.cc File src/kudu/tablet/tablet_replica_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/16581/4/src/kudu/tablet/tablet_replica_mm_ops.cc@264 PS4, Line 264: } > Nit: Perhaps update the docs in FlushOpPerfImprovementPolicy::SetPerfImprov Done -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 17:57:13 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16581 to look at the new patch set (#5). Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. KUDU-3195: flush when any DMS in the tablet is older than the time threshold Currently each tablet will wait at least 2 minutes between flushing DMSs, even if there are several DMSs that are older than 2 minutes. This means that for tablets with several dozen rowsets and updates across the entire tablet, it may take hours to flush all the deltas. Rather than waiting for 2 minutes since the last flush time before considering time-based flushing, this patch tracks the creation time of every DMS and flushes as long as there is a DMS that is older than 2 minutes in the tablet. Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tablet_server-test.cc 14 files changed, 219 insertions(+), 148 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/16581/5 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] wip KUDU-3149: don't block op registration on MM mutex
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/16580 )
Change subject: wip KUDU-3149: don't block op registration on MM mutex
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc@231
PS4, Line 231: void MaintenanceManager::RegisterOp(MaintenanceOp* op) {
Given this doesn't actually register the op, maybe we should rename the method
and add some docs that indicate it buffers the op and the op will be registered
in the scheduler thread.
http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc@291
PS4, Line 291: // Register any ops that may be pending registration.
Nit, this could be encapsulated in a method
http://gerrit.cloudera.org:8080/#/c/16580/4/src/kudu/util/maintenance_manager.cc@302
PS4, Line 302: emplace
> Probably, you meant to use insert() here? Would it even compile with emplac
It looks like emplace is valid C++11
http://www.cplusplus.com/reference/map/map/emplace/
--
To view, visit http://gerrit.cloudera.org:8080/16580
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a1b810f5b7ff6a22acc9b10b79ddffa8085c990
Gerrit-Change-Number: 16580
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Oct 2020 17:44:41 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/16581/4/src/kudu/tablet/tablet_replica_mm_ops.cc File src/kudu/tablet/tablet_replica_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/16581/4/src/kudu/tablet/tablet_replica_mm_ops.cc@264 PS4, Line 264: FlushOpPerfImprovementPolicy::SetPerfImprovementForFlush( Nit: Perhaps update the docs in FlushOpPerfImprovementPolicy::SetPerfImprovementForFlush given its usage of `elapsed_time` is was based on time since flush and is now based on the time since the earliest dms. -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 17:13:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 ) Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. Patch Set 4: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 17:09:33 + Gerrit-HasComments: No
[kudu-CR] [client] robust handling of absent master address(es)
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16589 ) Change subject: [client] robust handling of absent master address(es) .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 16:20:53 + Gerrit-HasComments: No
[kudu-CR] [client] robust handling of absent master address(es)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16589 ) Change subject: [client] robust handling of absent master address(es) .. Patch Set 2: Thank you for fast review, Grant! -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 16:18:55 + Gerrit-HasComments: No
[kudu-CR] [client] robust handling of absent master address(es)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16589 ) Change subject: [client] robust handling of absent master address(es) .. [client] robust handling of absent master address(es) This patch updates Kudu C++ client with robust handling of the absence of master addresses prior to connection attempt to a Kudu cluster. Before to this patch, KuduClientBuilder::Build() would hang in ConnectToCluster(). After this patch, KuduClientBuilder::Build() instantly returns Status::InvalidArgument() if no master addresses are provided. This patch also contains a test for the new behavior. Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Reviewed-on: http://gerrit.cloudera.org:8080/16589 Reviewed-by: Grant Henke Tested-by: Grant Henke --- M src/kudu/client/client-internal.cc M src/kudu/client/client-test.cc 2 files changed, 15 insertions(+), 0 deletions(-) Approvals: Grant Henke: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] robust handling of absent master address(es)
Grant Henke has removed a vote on this change. Change subject: [client] robust handling of absent master address(es) .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] robust handling of absent master address(es)
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16589 ) Change subject: [client] robust handling of absent master address(es) .. Patch Set 1: Verified+1 Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 14:49:58 + Gerrit-HasComments: No
[kudu-CR] [Java] Upgrade dependencies
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16583 ) Change subject: [Java] Upgrade dependencies .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/16583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7febccddbf080520ebdf6b4b71cc1571df1927b Gerrit-Change-Number: 16583 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 13 Oct 2020 14:45:35 + Gerrit-HasComments: No
[kudu-CR] [Java] Upgrade dependencies
Grant Henke has removed a vote on this change. Change subject: [Java] Upgrade dependencies .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: If7febccddbf080520ebdf6b4b71cc1571df1927b Gerrit-Change-Number: 16583 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3202: [build] Add Spark 3 Support
Hello Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16582 to look at the new patch set (#3). Change subject: KUDU-3202: [build] Add Spark 3 Support .. KUDU-3202: [build] Add Spark 3 Support This patch adds Sparks 3 support to the build and builds using Spark 3 by default while maintiaining the ability to build Spark 2. One simple change to the backup job was required to maintain maximum backwards compatibility due to SPARK-31404. Additionally a few small changes were needed to fix SpotBugs issues. Follow on changes will adjust build scripts and publishing docs to publish both Spark 2 and 3 Jars. Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f --- M java/config/spotbugs/excludeFilter.xml M java/gradle/dependencies.gradle M java/kudu-backup/src/main/scala/org/apache/kudu/backup/BackupUtils.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackup.scala M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 6 files changed, 42 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/16582/3 -- To view, visit http://gerrit.cloudera.org:8080/16582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I60d5afb1ce65d5bc7547f18ea3382bed6c71883f Gerrit-Change-Number: 16582 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Hello Tidy Bot, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16581 to look at the new patch set (#4). Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. KUDU-3195: flush when any DMS in the tablet is older than the time threshold Currently each tablet will wait at least 2 minutes between flushing DMSs, even if there are several DMSs that are older than 2 minutes. This means that for tablets with several dozen rowsets and updates across the entire tablet, it may take hours to flush all the deltas. Rather than waiting for 2 minutes since the last flush time before considering time-based flushing, this patch tracks the creation time of every DMS and flushes as long as there is a DMS that is older than 2 minutes in the tablet. Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tablet_server-test.cc 14 files changed, 211 insertions(+), 142 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/16581/4 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] robust handling of absent master address(es)
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16589 Change subject: [client] robust handling of absent master address(es) .. [client] robust handling of absent master address(es) This patch updates Kudu C++ client with robust handling of the absence of master addresses prior to connection attempt to a Kudu cluster. Before to this patch, KuduClientBuilder::Build() would hang in ConnectToCluster(). After this patch, KuduClientBuilder::Build() instantly returns Status::InvalidArgument() if no master addresses are provided. This patch also contains a test for the new behavior. Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-test.cc 2 files changed, 15 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/16589/1 -- To view, visit http://gerrit.cloudera.org:8080/16589 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4c7400d09298c01cd0b349cc2b9f481d1214f148 Gerrit-Change-Number: 16589 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-3195: flush when any DMS in the tablet is older than the time threshold
Hello Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16581 to look at the new patch set (#3). Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold .. KUDU-3195: flush when any DMS in the tablet is older than the time threshold Currently each tablet will wait at least 2 minutes between flushing DMSs, even if there are several DMSs that are older than 2 minutes. This means that for tablets with several dozen rowsets and updates across the entire tablet, it may take hours to flush all the deltas. Rather than waiting for 2 minutes since the last flush time before considering time-based flushing, this patch tracks the creation time of every DMS and flushes as long as there is a DMS that is older than 2 minutes in the tablet. Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 --- M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tablet_server-test.cc 14 files changed, 184 insertions(+), 111 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/16581/3 -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
