[kudu-CR] KUDU-613: Introduce new BlockCache metrics
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21389 ) Change subject: KUDU-613: Introduce new BlockCache metrics .. Patch Set 3: Build Aborted http://jenkins.kudu.apache.org/job/pre_commit/132/ : ABORTED -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Sat, 11 May 2024 04:54:12 + Gerrit-HasComments: No
[kudu-CR] KUDU-613: Introduce SLRU cache
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 ) Change subject: KUDU-613: Introduce SLRU cache .. Patch Set 17: Build Aborted http://jenkins.kudu.apache.org/job/pre_commit/131/ : ABORTED -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 17 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 11 May 2024 04:54:11 + Gerrit-HasComments: No
[kudu-CR] Leader rebalance ignores soft deleted tables
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/21411 )
Change subject: Leader rebalance ignores soft deleted tables
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/21411/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/21411/1//COMMIT_MSG@10
PS1, Line 10: no needs too leader rebalance
nit: not needed to do leader rebalance for
http://gerrit.cloudera.org:8080/#/c/21411/1//COMMIT_MSG@12
PS1, Line 12: provide
provides
http://gerrit.cloudera.org:8080/#/c/21411/1//COMMIT_MSG@12
PS1, Line 12: path
patch
http://gerrit.cloudera.org:8080/#/c/21411/1//COMMIT_MSG@12
PS1, Line 12: configure
configuration
http://gerrit.cloudera.org:8080/#/c/21411/1/src/kudu/master/auto_leader_rebalancer.cc
File src/kudu/master/auto_leader_rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/21411/1/src/kudu/master/auto_leader_rebalancer.cc@90
PS1, Line 90: leader_rebalancing_filter_soft_deleted_tables
How about 'leader_rebalancing_ignore_soft_deleted_tables' to clarify?
http://gerrit.cloudera.org:8080/#/c/21411/1/src/kudu/master/auto_leader_rebalancer.cc@435
PS1, Line 435: catalog_manager_->GetAllTables(_infos);
GetAllTables returns a vector related to 'table_ids_map_', when 'normal' tables
are needed, how about returning a vector related to
'normalized_table_names_map_' which only contians non-soft-deleted tables. You
can add a function for the purpose.
http://gerrit.cloudera.org:8080/#/c/21411/1/src/kudu/master/auto_leader_rebalancer.cc@437
PS1, Line 437: for (auto iter = table_infos.begin(); iter != table_infos.end();
iter++) {
nit: for (auto table_info : table_infos) { ?
--
To view, visit http://gerrit.cloudera.org:8080/21411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e2f37b004ed7d394e18d21cce97a2c9702adba3
Gerrit-Change-Number: 21411
Gerrit-PatchSet: 1
Gerrit-Owner: Wang Xixu <1450306...@qq.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Sat, 11 May 2024 04:21:05 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add metrics for tablet copy op time
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/21356 )
Change subject: [metrics] Add metrics for tablet copy op time
..
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21356/6/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/21356/6/src/kudu/tserver/tablet_copy_client.h@197
PS6, Line 197: const tablet::TabletMetrics* TabletMetrics() { return
tablet_metrics_; }
Is this necessary? It seems TabletMetrics() is only used in TabletCopyClient
class, we can access tablet_metrics_ directly.
--
To view, visit http://gerrit.cloudera.org:8080/21356
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I088f6a9a8a07ad39ca95ae8b4995ce00d1a0d00c
Gerrit-Change-Number: 21356
Gerrit-PatchSet: 6
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Sat, 11 May 2024 03:18:52 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add tablet level metrics for alter schema op time
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/21300 )
Change subject: [metrics] Add tablet level metrics for alter schema op time
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21300/4/src/kudu/tablet/ops/alter_schema_op.cc
File src/kudu/tablet/ops/alter_schema_op.cc:
http://gerrit.cloudera.org:8080/#/c/21300/4/src/kudu/tablet/ops/alter_schema_op.cc@158
PS4, Line 158: auto update_metrics = [this]() {
How about making it as a ScopedCleanup, then you don't need to call it manually
in every return.
--
To view, visit http://gerrit.cloudera.org:8080/21300
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I034fc3141349a940ee8aaac22ca90e1948fc7a6a
Gerrit-Change-Number: 21300
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Sat, 11 May 2024 03:10:59 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add metrics for participant op time
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/21375 )
Change subject: [metrics] Add metrics for participant op time
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21375/1/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/21375/1/src/kudu/tablet/ops/participant_op.cc@280
PS1, Line 280: auto update_metrics = [this]() {
How about making it as a ScopedCleanup, then you don't need to call it manually
in every return.
http://gerrit.cloudera.org:8080/#/c/21375/1/src/kudu/tablet/txn_participant-test.cc
File src/kudu/tablet/txn_participant-test.cc:
http://gerrit.cloudera.org:8080/#/c/21375/1/src/kudu/tablet/txn_participant-test.cc@248
PS1, Line 248: a
nit: an
--
To view, visit http://gerrit.cloudera.org:8080/21375
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie25d4bee27e9ba35925634b61fb518467bb59201
Gerrit-Change-Number: 21375
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Sat, 11 May 2024 02:59:46 +
Gerrit-HasComments: Yes
[kudu-CR] [metrics] Add tablet level metrics for scans op time
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21291 ) Change subject: [metrics] Add tablet level metrics for scans op time .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/21291/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/21291/3/src/kudu/tserver/tablet_server-test.cc@2290 PS3, Line 2290: ASSERT_FALSE(wall_tmp == nit: ASSERT_NE or ASSERT_GT Similar to the following 3 ASSERTs. http://gerrit.cloudera.org:8080/#/c/21291/3/src/kudu/tserver/tablet_server-test.cc@2317 PS3, Line 2317: Do you mind to add some comments to describe the purpose of each newly added tests? -- To view, visit http://gerrit.cloudera.org:8080/21291 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f490cfb6f37aef60b34697100fb502374fcc503 Gerrit-Change-Number: 21291 Gerrit-PatchSet: 3 Gerrit-Owner: KeDeng Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Sat, 11 May 2024 02:48:42 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce SLRU cache
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache
..
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc
File src/kudu/util/slru_cache.cc:
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@543
PS17, Line 543: // Release from either the probationary or the protected
shard.
: if (!e->protected_segment) {
: probationary_shard_->Release(handle);
: } else {
: protected_shard_->Release(handle);
: }
: }
What guards against the following scenarios:
Scenario_0:
* thread_A: an instance of Cache::UniqueHandle as a wrapper for entry_X is
being destroyed
* thread_A: while executing SLRUCacheShardPair::Release(), finds entry_X
isn't in the protected shard
* thread_B: calls SLRUCacheShardPair::Lookup(), moving entry_X into the
protected shard
* thread_A: calls Release() on the corresponding probationary shard for entry_X
Scenario_1:
* thread_A: an instance of Cache::UniqueHandle as a wrapper for entry_X is
being destroyed
* thread_A: while executing SLRUCacheShardPair::Release(), finds entry_X is in
the protected shard
* thread_B: calls SLRUCacheShardPair::Lookup() for some other entry_Y which is
in the probationary shard, but due to crossing the threshold of lookup
operations per entry, it's now being moved into the protected shard, so entry_X
is pushed out of the protected shard
* thread_A: calls Release() on the corresponding protected shard for entry_X
If nothing prevents from having such scenarios, then a few questions:
* Does the contents SLRU cache stay consistent after that?
* Do metrics of the SLRU cache stay consistent after that?
--
To view, visit http://gerrit.cloudera.org:8080/20607
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb
Gerrit-Change-Number: 20607
Gerrit-PatchSet: 17
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Marton Greber
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 11 May 2024 01:09:24 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce SLRU cache
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache
..
Patch Set 16:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.cc
File src/kudu/util/slru_cache.cc:
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.cc@254
PS16, Line 254: e = table_.Lookup(key, hash);
What guarantees do we have to be sure that 'table_' cannot be modified under
the hood by concurrently running SLRUCacheShard::Insert()? It would be great
to add a note about that.
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.cc@267
PS16, Line 267: e = table_.Lookup(key, hash);
ditto: what if Insert() is running concurrently and modifying the 'table_' --
what protects us from such concurrency issues? Would be great to add a note in
the code.
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc
File src/kudu/util/slru_cache.cc:
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@273
PS17, Line 273: bool last_reference = Unref(e);
: if (last_reference) {
nit: could these be joined, adding a comment on what's the intention of this
code?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@471
PS17, Line 471: {
What is the purpose of this extra scope?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@501
PS17, Line 501: {
What is the purpose of this extra scope?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@552
PS17, Line 552: {
What is the purpose of this extra scope?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@561
PS17, Line 561: {
What is the purpose of this extra scope?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@581
PS17, Line 581:
nit:
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@601
PS17, Line 601: // Similar to STLDeleteElements().
: auto begin = shards_.begin();
: while (begin != shards_.end()) {
: auto temp = begin;
: ++begin;
: temp->reset(nullptr);
: }
Is this really needed? Wouldn't the contained unique_ptr be destroyed
automatically upon the destruction of the containing vector?
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@694
PS17, Line 694: uint8_t* data = reinterpret_cast(h);
: delete [] data;
nit: could these lines be joined?
--
To view, visit http://gerrit.cloudera.org:8080/20607
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb
Gerrit-Change-Number: 20607
Gerrit-PatchSet: 16
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Reviewer: Marton Greber
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 10 May 2024 21:33:01 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce SLRU cache
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 ) Change subject: KUDU-613: Introduce SLRU cache .. Patch Set 17: (1 comment) http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/cache.h File src/kudu/util/cache.h: http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/cache.h@520 PS17, Line 520: // Returns true if probationary segment contains key. : // Returns false if not. : // Only used for SLRU cache. : virtual bool ProbationaryContains(const Slice& key) = 0; : : // Returns true if protected segment contains key. : // Returns false if not. : // Only used for SLRU cache. : virtual bool ProtectedContains(const Slice& key) = 0; > Similar to SetSegmentMetrics(), this doesn't seem to be a place for these m SLRUCacheShard is the right place for these, even without any extras, I don't see any reason to keep these specific methods in this generic interface (but I might be missing something). -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 17 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 10 May 2024 20:57:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce SLRU cache
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache
..
Patch Set 17:
(16 comments)
A few more high-level observations.
http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20607/16//COMMIT_MSG@22
PS16, Line 22: Lookups/sec
It would be nice to mentions the following details:
* What type of build it was?
* What sort of machine it was: CPU cores and frequency?
It's crucial to measure this for RELEASE builds, we aren't interested to look
at other types of builds here, I guess.
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache-bench.cc
File src/kudu/util/cache-bench.cc:
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache-bench.cc@92
PS16, Line 92: ret+= " SLRU";
nit for here and elsewhere: separate variable and the operator by space
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/cache.h@520
PS17, Line 520: // Returns true if probationary segment contains key.
: // Returns false if not.
: // Only used for SLRU cache.
: virtual bool ProbationaryContains(const Slice& key) = 0;
:
: // Returns true if protected segment contains key.
: // Returns false if not.
: // Only used for SLRU cache.
: virtual bool ProtectedContains(const Slice& key) = 0;
Similar to SetSegmentMetrics(), this doesn't seem to be a place for these
methods.
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache.h@58
PS16, Line 58: Protects against random/long reads polluting cache.
nit: consider either making this part of the comment more generic (as it should
be at this level) or dropping this part altogether
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache.h@91
PS16, Line 91: protected_segment
nit: should be rather named 'in_protected_segment'?
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/cache.h@116
PS16, Line 116: sanitize
style nit: since this isn't an accessor-like method, its name should be
capitalized
http://gerrit.cloudera.org:8080/#/c/20607/14/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/20607/14/src/kudu/util/cache.h@228
PS14, Line 228: // ensure this is safe, e.g. by destructing the entity that
owned the
: // original metrics.
: kReset,
: };
: // Set the cache metrics to update corresponding counters
accordingly.
: virtual void SetMetrics(std::unique_ptr metrics,
: ExistingMetricsPolicy metrics_policy)
= 0;
:
: // Sets probationary and protected segments metrics to update
corresponding counters accordingly.
: //
: // An upgrade of one entry will increment the number of
evictions from the probationary segment by
: // one and the number of insertions into the protected
segment by one.
: //
: // A downgrade of one entry will increment the number of
evictions from the protected segment by
: // one and the number of insertions into the probationary
segment by one.
: //
: // To calculate the number of inserts for the SLRU cache,
subtract the number of evictions
: // of the protected segment from the number of inserts of the
probationary segment.
: //
> This will be used in the block cache to set the metrics for both segments o
OK. But even with that, instead of polluting the interface of the base Cache
class you could make the BlockCache class to be a template by the type of the
underlying cache implementation, and remove both SetMetrics() and
SetSegmentMetrics() from the common Cache interface.
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/nvm_cache.h
File src/kudu/util/nvm_cache.h:
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/nvm_cache.h@46
PS16, Line 46: // Number of lookups for this entry. Used for upgrading to
protected segment in SLRU cache.
: // Resets to 0 when moved between probationary and protected
segments in both directions.
: uint32_t lookups;
Is this really needed? I didn't see NVM-based implementation of SLRU cache, so
this looks a bit strange given you decided not to converge on {S}LRUHandle
structure
http://gerrit.cloudera.org:8080/#/c/20607/16/src/kudu/util/slru_cache.h
File src/kudu/util/slru_cache.h:
[kudu-CR] KUDU-613: Introduce SLRU cache
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 ) Change subject: KUDU-613: Introduce SLRU cache .. Patch Set 17: (2 comments) http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache-test.cc@413 PS17, Line 413: evicted to probationary segment If the probationary segment doesn't have enough free space for this new entry, the least recently accessed entry will be evicted to free up space - is that correct? If so, does it make sense to have a test case for that? http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/17/src/kudu/util/slru_cache.cc@490 PS17, Line 490: and nit: delete 'and' -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 17 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 10 May 2024 20:38:10 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Integrate SLRU cache into block cache
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/21390 )
Change subject: KUDU-613: Integrate SLRU cache into block cache
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc
File src/kudu/cfile/block_cache.cc:
http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@235
PS2, Line 235: BlockCache::BlockCache()
: : BlockCache(FLAGS_block_cache_capacity_mb * 1024 * 1024) {
: }
Interesting, but why this constructor isn't updated to take care of different
type of the underlying cache? That's especially strange given the funny code
below for the next constructor when the 'capacity' parameter is ignored upon
using the SLRU type of the cache.
http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@242
PS2, Line 242:
CreateCache(FLAGS_block_cache_probationary_segment_capacity_mb * 1024 * 1024,
:
FLAGS_block_cache_protected_segment_capacity_mb * 1024 * 1024,
:
FLAGS_block_cache_lookups_before_upgrade)) {
That's a bad smell -- how 'capacity' is then used in this branch?
http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@247
PS2, Line 247: BlockCache::BlockCache(size_t probationary_segment_capacity,
:size_t protected_segment_capacity,
:size_t lookups)
: : cache_(CreateCache(probationary_segment_capacity,
protected_segment_capacity, lookups)) {
: }
With this code and the code above it smells even worse -- now there is a
complete mess with trying to understand when a particular type of cache is
created based on the constructor invoked and the --block_cache_eviction_policy
setting.
--
To view, visit http://gerrit.cloudera.org:8080/21390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I04411ab2756045f15a272f3397d46d871b087b03
Gerrit-Change-Number: 21390
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy
Gerrit-Comment-Date: Fri, 10 May 2024 18:29:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Integrate SLRU cache into block cache
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21390 ) Change subject: KUDU-613: Integrate SLRU cache into block cache .. Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/21390/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21390/2//COMMIT_MSG@13 PS2, Line 13: The default : value is still the old LRU cache. I think it's crucial to enable at least a few integration-style scenarios using the block cache backed by the new SLRU implementation. I'd think of scenarios in tablet_server_stress-test.cc, full_stack-insert-scan-test.cc, dense_node-itest.cc and heavy-update-compaction-itest.cc http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc File src/kudu/cfile/block_cache.cc: http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@46 PS2, Line 46: Either 'LRU' (default) or 'SLRU' (experimental Please add a validator for the value, and make this flag case-insensitive, so "LRU" and "lru" both would work (as well as "lRu"). http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@46 PS2, Line 46: nit: drop the trailing space? http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@49 PS2, Line 49: block_cache_probationary_segment_capacity_mb For this the block_cache_protected_segment_capacity_mb flag below: Does it make sense to add a validator for this flag to reject at least negative values? From the other side, why not to use unsigned type for this flag? http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@50 PS2, Line 50: MB here and below: is this actually MiB/MiBytes? http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@58 PS2, Line 58: ," nit: reconsider the punctuation and the result message for the flag http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@171 PS2, Line 171: "Lower --block_cache_probationary_segment_capacity_mb and" nit: add space after 'and' http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@194 PS2, Line 194: FLAGS_block_cache_eviction_policy Please make sure the value of this flag is treated as case-insensitive. Doing that in just one place and then reusing the function that converts a string into a enumeration should be robust enough. http://gerrit.cloudera.org:8080/#/c/21390/2/src/kudu/cfile/block_cache.cc@222 PS2, Line 222: ToUpperCase(FLAGS_block_cache_eviction_policy, _block_cache_eviction_policy); I'm not sure it's a good idea to update the actual value of the flag like this: it might quite unexpected behavior from the user's perspective. Why not to use a temporary variable for converting to the upper case? -- To view, visit http://gerrit.cloudera.org:8080/21390 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I04411ab2756045f15a272f3397d46d871b087b03 Gerrit-Change-Number: 21390 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Fri, 10 May 2024 17:56:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce new BlockCache metrics
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21389 ) Change subject: KUDU-613: Introduce new BlockCache metrics .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/21389/2/src/kudu/util/block_cache_metrics.cc File src/kudu/util/block_cache_metrics.cc: PS2: > The new metrics give us information on inserts into the protected segment o >From the usability perspective, I'd think of all the metrics that allow for >computing SLRU cache's hit and miss ratios and show how many inserts and >evictions happened. That's what end-user is about to look at. Yes, most >likely it's possible to deduce that from counters available for the >probational and protected segments (but I haven't verified that), but where >one can find the rules to perform such computations and how many mistakes a >person is going to make when trying to compute those? I'd think of providing top-level metrics for the SLRU cache as we currently have for the LRU cache just for better usability. http://gerrit.cloudera.org:8080/#/c/21389/2/src/kudu/util/block_cache_metrics.cc@176 PS2, Line 176: nit for here and below: this isn't aligned with current Kudu style guide -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Fri, 10 May 2024 17:50:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce new BlockCache metrics
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21389 ) Change subject: KUDU-613: Introduce new BlockCache metrics .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/21389/2/src/kudu/util/block_cache_metrics.cc File src/kudu/util/block_cache_metrics.cc: PS2: > Do we need to also have metrics for entries moving between segments? The new metrics give us information on inserts into the protected segment of the block cache (block_cache_protected_segment_inserts). That lets us know how many entries were upgraded from the probationary segment to the protected segment. They also give us information on evictions from the protected segment of the block cache (block_cache_protected_segment_evictions). This lets us know how many entries were downgraded from the protected segment to the probationary segment. Are there specific metrics that are currently missing that you had in mind? -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Fri, 10 May 2024 16:54:53 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Introduce new BlockCache metrics
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21389 ) Change subject: KUDU-613: Introduce new BlockCache metrics .. Patch Set 3: Build Started http://jenkins.kudu.apache.org/job/pre_commit/132/ -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Comment-Date: Fri, 10 May 2024 16:54:10 + Gerrit-HasComments: No
[kudu-CR] KUDU-613: Introduce new BlockCache metrics
Hello Kudu Jenkins, Abhishek Chennaka, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21389 to look at the new patch set (#3). Change subject: KUDU-613: Introduce new BlockCache metrics .. KUDU-613: Introduce new BlockCache metrics With the new SLRU cache used in the block cache, new metrics are needed for both the probationary and the protected segment. Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d --- M src/kudu/util/block_cache_metrics.cc M src/kudu/util/block_cache_metrics.h 2 files changed, 133 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/21389/3 -- To view, visit http://gerrit.cloudera.org:8080/21389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1c8181ec6bea301605aaef7db0003c3eaef3072d Gerrit-Change-Number: 21389 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
[kudu-CR] KUDU-613: Introduce SLRU cache
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 ) Change subject: KUDU-613: Introduce SLRU cache .. Patch Set 17: Build Started http://jenkins.kudu.apache.org/job/pre_commit/131/ -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 17 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 10 May 2024 16:54:10 + Gerrit-HasComments: No
[kudu-CR] [master] fix race in auto leader rebalancing
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21417 ) Change subject: [master] fix race in auto leader rebalancing .. [master] fix race in auto leader rebalancing It turned out that auto leader rebalancing task wasn't explicitly shutdown upon shutting down catalog manager. That lead to race conditions as reported by TSAN, at least in test scenarios (see below). This patch addresses the issue. WARNING: ThreadSanitizer: data race (pid=23827) Write of size 1 at 0x7b408208 by main thread: #0 AnnotateRWLockDestroy thirdparty/src/llvm-11.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp:264 (auto_rebalancer-test+0x33575e) #1 kudu::rw_spinlock::~rw_spinlock() src/kudu/util/locks.h:89:5 (libmaster.so+0x359376) #2 kudu::master::TSManager::~TSManager() src/kudu/master/ts_manager.cc:108:1 (libmaster.so+0x4ad201) #3 kudu::master::TSManager::~TSManager() src/kudu/master/ts_manager.cc:107:25 (libmaster.so+0x4ad229) #4 std::__1::default_delete::operator()(kudu::master::TSManager*) const thirdparty/installed/tsan/include/c++/v1/memory:2262:5 (libmaster.so+0x407ce7) #5 std::__1::unique_ptr >::reset(kudu::master::TSManager*) thirdparty/installed/tsan/include/c++/v1/memory:2517:7 (libmaster.so+0x40157d) #6 std::__1::unique_ptr >::~unique_ptr() thirdparty/installed/tsan/include/c++/v1/memory:2471:19 (libmaster.so+0x4015eb) #7 kudu::master::Master::~Master() src/kudu/master/master.cc:263:1 (libmaster.so+0x3f7a4a) #8 kudu::master::Master::~Master() src/kudu/master/master.cc:261:19 (libmaster.so+0x3f7dc9) #9 std::__1::default_delete::operator()(kudu::master::Master*) const thirdparty/installed/tsan/include/c++/v1/memory:2262:5 (libmaster.so+0x435627) #10 std::__1::unique_ptr >::reset(kudu::master::Master*) thirdparty/installed/tsan/include/c++/v1/memory:2517:7 (libmaster.so+0x42e6ed) #11 kudu::master::MiniMaster::Shutdown() src/kudu/master/mini_master.cc:120:13 (libmaster.so+0x4c2612) ... Previous atomic write of size 4 at 0x7b408208 by thread T439 (mutexes: write M1141235379631443968): #0 __tsan_atomic32_compare_exchange_strong thirdparty/src/llvm-11.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cpp:780 (auto_rebalancer-test+0x33eb60) #1 base::subtle::Release_CompareAndSwap(int volatile*, int, int) /src/kudu/gutil/atomicops-internals-tsan.h:88:3 (libmaster.so+0x2e2b34) #2 kudu::rw_semaphore::unlock_shared() src/kudu/util/rw_semaphore.h:91:19 (libmaster.so+0x2e29c8) #3 kudu::rw_spinlock::unlock_shared() src/kudu/util/locks.h:99:10 (libmaster.so+0x2e28ef) #4 std::__1::shared_lock::~shared_lock() /thirdparty/installed/tsan/include/c++/v1/shared_mutex:369:19 (libmaster.so+0x2e23e0) #5 kudu::master::TSManager::GetAllDescriptors(std::__1::vector, std::__1::allocator > >*) const src/kudu/master/ts_manager.cc:206:1 (libmaster.so+0x4adeb6) #6 kudu::master::AutoLeaderRebalancerTask::RunLeaderRebalancer() src/kudu/master/auto_leader_rebalancer.cc:405:16 (libmaster.so+0x2fb51b) #7 kudu::master::AutoLeaderRebalancerTask::RunLoop() src/kudu/master/auto_leader_rebalancer.cc:445:7 (libmaster.so+0x2fbaa9) This is a follow-up to 10efaf2c77dfe5e4474505e0267c583c011703be. Change-Id: Iccd66d00280d22b37386230874937e5260f07f3b Reviewed-on: http://gerrit.cloudera.org:8080/21417 Reviewed-by: Wang Xixu <1450306...@qq.com> Tested-by: Alexey Serbin Reviewed-by: Yifan Zhang --- M src/kudu/master/auto_leader_rebalancer.cc M src/kudu/master/catalog_manager.cc 2 files changed, 9 insertions(+), 1 deletion(-) Approvals: Wang Xixu: Looks good to me, but someone else must approve Alexey Serbin: Verified Yifan Zhang: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/21417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iccd66d00280d22b37386230874937e5260f07f3b Gerrit-Change-Number: 21417 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21385 ) Change subject: [glog] Fix minidump-test for cold symbols. .. Patch Set 3: Verified+1 Build Successful http://jenkins.kudu.apache.org/job/pre_commit/130/ : SUCCESS -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 May 2024 14:58:42 + Gerrit-HasComments: No
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21385 to look at the new patch set (#2). Change subject: [glog] Fix minidump-test for cold symbols. .. [glog] Fix minidump-test for cold symbols. glog 0.6.0 cannot reliably resolve cold symbols. This was only fixed in recent versions: https://github.com/google/glog/issues/869 This might cause ASSERT_DEATH checks to fail in minidump-test. The long term solution would be to updata to 0.7.0: https://issues.apache.org/jira/browse/KUDU-3572 One affected platform is RHEL 9.3 with default g++. Change-Id: I18d88790d09e9763223a767617794332859cbfad --- M src/kudu/util/minidump-test.cc 1 file changed, 10 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/21385/2 -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21385 ) Change subject: [glog] Fix minidump-test for cold symbols. .. Patch Set 3: Build Started http://jenkins.kudu.apache.org/job/pre_commit/130/ -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 May 2024 14:07:29 + Gerrit-HasComments: No
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21385 to look at the new patch set (#3). Change subject: [glog] Fix minidump-test for cold symbols. .. [glog] Fix minidump-test for cold symbols. glog 0.6.0 cannot reliably resolve cold symbols. This was only fixed in recent versions: https://github.com/google/glog/issues/869 This might cause ASSERT_DEATH checks to fail in minidump-test. The long term solution would be to update to 0.7.0: https://issues.apache.org/jira/browse/KUDU-3572 One affected platform is RHEL 9.3 with default g++. Change-Id: I18d88790d09e9763223a767617794332859cbfad --- M src/kudu/util/minidump-test.cc 1 file changed, 10 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/21385/3 -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21385 ) Change subject: [glog] Fix minidump-test for cold symbols. .. Patch Set 2: Build Aborted http://jenkins.kudu.apache.org/job/pre_commit/129/ : ABORTED -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 May 2024 14:07:23 + Gerrit-HasComments: No
[kudu-CR] [glog] Fix minidump-test for cold symbols.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21385 ) Change subject: [glog] Fix minidump-test for cold symbols. .. Patch Set 2: Build Started http://jenkins.kudu.apache.org/job/pre_commit/129/ -- To view, visit http://gerrit.cloudera.org:8080/21385 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18d88790d09e9763223a767617794332859cbfad Gerrit-Change-Number: 21385 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Martonka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 May 2024 14:06:09 + Gerrit-HasComments: No
[kudu-CR] [security-flags-itest] Fix missing command line flags
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21399 ) Change subject: [security-flags-itest] Fix missing command line flags .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/21399/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21399/2//COMMIT_MSG@7 PS2, Line 7: [security-flags-itest] Fix missing command line flags > Could give some description about this problem? Done http://gerrit.cloudera.org:8080/#/c/21399/2/src/kudu/integration-tests/security-flags-itest.cc File src/kudu/integration-tests/security-flags-itest.cc: http://gerrit.cloudera.org:8080/#/c/21399/2/src/kudu/integration-tests/security-flags-itest.cc@52 PS2, Line 52: This happened on Ubuntu 22.04 : // release build. > Could you create an issue to describe the error stack? I expanded the explanation to make it clearer. -- To view, visit http://gerrit.cloudera.org:8080/21399 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec751e8761562612d97b886740c9b20cd134a0bc Gerrit-Change-Number: 21399 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anonymous Coward (763) Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Fri, 10 May 2024 12:32:38 + Gerrit-HasComments: Yes
[kudu-CR] [wip][build] KUDU-3551 Upgrade gradle to 7.6.4
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21030 ) Change subject: [wip][build] KUDU-3551 Upgrade gradle to 7.6.4 .. Patch Set 14: Verified-1 Build Failed http://jenkins.kudu.apache.org/job/pre_commit/128/ : FAILURE -- To view, visit http://gerrit.cloudera.org:8080/21030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I915dab011aba633d55a79c72ea6f459d703d7f47 Gerrit-Change-Number: 21030 Gerrit-PatchSet: 14 Gerrit-Owner: Zoltan Chovan Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 10 May 2024 09:55:19 + Gerrit-HasComments: No
[kudu-CR] [master] fix race in auto leader rebalancing
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21417 ) Change subject: [master] fix race in auto leader rebalancing .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/21417 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iccd66d00280d22b37386230874937e5260f07f3b Gerrit-Change-Number: 21417 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 10 May 2024 09:07:07 + Gerrit-HasComments: No
[kudu-CR] [wip][build] KUDU-3551 Upgrade gradle to 7.6.4
Hello Marton Greber, Alexey Serbin, Zoltan Martonka, Attila Bukor, Kudu Jenkins, Abhishek Chennaka, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/21030 to look at the new patch set (#14). Change subject: [wip][build] KUDU-3551 Upgrade gradle to 7.6.4 .. [wip][build] KUDU-3551 Upgrade gradle to 7.6.4 * Gradle version has been upgraded to 7.6.4 from 6.8.3, both in the dependencies declaration and the actual wrapper code itself (/java/gradlew) * Removed jcenter from list of repositories - https://docs.gradle.org/6.9.2/userguide/upgrading_version_6.html?_gl=1*1xt1kk*_ga*MjQ4NTk5ODI5LjE2ODc0MDc0OTA.*_ga_7W7NC6YNPT*MTcwNzEyMzc1Ni4zLjEuMTcwNzEyODU2MS41NC4wLjA.#jcenter_deprecation * Replaced compile and testCompile configurations with implementation and testImplementation respectively - https://docs.gradle.org/6.9.2/userguide/upgrading_version_5.html?_gl=1*1ldn5ls*_ga*MjQ4NTk5ODI5LjE2ODc0MDc0OTA.*_ga_7W7NC6YNPT*MTcwNzEyMzc1Ni4zLjEuMTcwNzEyODY1My40NS4wLjA.#dependencies_should_no_longer_be_declared_using_the_compile_and_runtime_configurations * Replaced extension methods with archiveExtension - https://docs.gradle.org/6.9.2/dsl/org.gradle.api.tasks.bundling.AbstractArchiveTask.html?_gl=1*1ldn5ls*_ga*MjQ4NTk5ODI5LjE2ODc0MDc0OTA.*_ga_7W7NC6YNPT*MTcwNzEyMzc1Ni4zLjEuMTcwNzEyODY1My40NS4wLjA.#org.gradle.api.tasks.bundling.AbstractArchiveTask:extension * Removed scopes.gradle and the propdeps plugin as optional and provided scopes are no longer supported/working in Gradle7, these have been replaced by either compileOnly * Upgraded scalafmt version - https://github.com/alenkacz/gradle-scalafmt/issues/39 * Pinned the scalafmt config to version 1.5.1 to avoid the new formatting errors that popped up with the new scalafmt version * Upgraded Spark3 version to 3.2.4 to solve Guava related compilation errors (some classes have been deprecated and removed, which led to compilation failure) * Added LogCaptor to resolve issues with CapturingLogAppender, which stopped working in the Scala tests, fixing was taking too much time so this was implemented as a workaround, it only affected 4 tests in TestKuduBackup.scala, if the CLA errors are fixed later, the dependency can be removed and the test changes reverted. * As the legacy maven plugin was removed, the signing and publishing has been rewritten using the new maven-publish plugin. I tested the signing and publishing with a locally hosted Maven repository, however this needs further testing and confirmation from a commiter to ensure it works as expected. The new plugin uses a different task to publish the artifacts (publish instead of uploadArchives) an alias was created to keep backwards compatibility with existing docs/scripts. * The permissions for assign-location.py script were updated, as previously the permissions of it's symlink were copied, but that changed in the new gradle version and the original (non executable) permissions led to test failures. * The shadow plugin ran into a known error which was fixed as recommended - https://docs.gradle.org/7.6.4/dsl/org.gradle.api.tasks.bundling.Zip.html#org.gradle.api.tasks.bundling.Zip:zip64 * Two tasks were updated in the shadow.gradle file as well, this needs another round of verification after the jars are built correctly * Dependency scopes were only modified when necessary (compilation failure) Still existing problem: * The jars generated currently do not pass the verify\_jars.pl script as there's a bunch of extra classes and files packaged into them than previously. The only reason that I was able to find for this is that the sources collected for the artifacts are determined differently in gradle 7 * The extensive dependency exclusions that can be seen are the quick and dirty fixes, I'm not happy with this solution but I've yet to find another, recommendations and comment are welcome on this part. Change-Id: I915dab011aba633d55a79c72ea6f459d703d7f47 --- M java/.scalafmt.conf M java/build.gradle M java/buildSrc/build.gradle M java/gradle/artifacts.gradle M java/gradle/dependencies.gradle M java/gradle/publishing.gradle M java/gradle/quality.gradle D java/gradle/scopes.gradle M java/gradle/shadow.gradle M java/gradle/wrapper/gradle-wrapper.properties M java/gradlew M java/kudu-backup-common/build.gradle M java/kudu-backup-tools/build.gradle M java/kudu-backup/build.gradle M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala M java/kudu-client/build.gradle M java/kudu-hive/build.gradle M java/kudu-jepsen/build.gradle M java/kudu-proto/build.gradle M java/kudu-spark-tools/build.gradle M java/kudu-spark/build.gradle M java/kudu-subprocess/build.gradle M java/kudu-test-utils/build.gradle M
[kudu-CR] [wip][build] KUDU-3551 Upgrade gradle to 7.6.4
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21030 ) Change subject: [wip][build] KUDU-3551 Upgrade gradle to 7.6.4 .. Patch Set 14: Build Started http://jenkins.kudu.apache.org/job/pre_commit/128/ -- To view, visit http://gerrit.cloudera.org:8080/21030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I915dab011aba633d55a79c72ea6f459d703d7f47 Gerrit-Change-Number: 21030 Gerrit-PatchSet: 14 Gerrit-Owner: Zoltan Chovan Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: Zoltan Martonka Gerrit-Comment-Date: Fri, 10 May 2024 09:00:13 + Gerrit-HasComments: No
