You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mahesh Reddy (Code Review)" <ge...@cloudera.org> on 2024/10/18 20:39:10 UTC
[kudu-CR] KUDU-613: Fix minor metrics related bug
Mahesh Reddy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21956
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug so now
the metrics should be accurate.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
---
M src/kudu/util/slru_cache.cc
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/21956/1
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/627/
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:57:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc
File src/kudu/util/slru_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@302
PS3, Line 302:
> Yep that works, changed it.
Thanks! This way it looks less error prone and less suspicious by keeping the metrics allocated under their original owner.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 23:08:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3:
> Patch Set 3: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/pre_commit/616/ : FAILURE
All test failures are unrelated to the change/known flaky tests.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 20:47:12 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/604/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2024 22:33:46 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc
File src/kudu/util/slru_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@301
PS3, Line 301: nter
> nit: consider using down_cast<> and asserting on the non-null of the result
Done
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@302
PS3, Line 302: ld is now null.
> IIUC, this removes/nullifies the metrics from under the slru_cache_ instanc
That was my reservation about this approach but I haven't found another way to access the segment-level metrics in the test. Given it's a unique pointer, it must be released to be accessed here. I guess it's ok to do so since this is a test. I'll add a comment in the test about this behavior.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 20:27:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 2: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/615/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 19:13:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 23:05:38 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 4:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/625/
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 20:27:14 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21956/1//COMMIT_MSG
Commit Message:
PS1:
Do you mind adding a unit test to cover this? That should help to catch regressions in future updates, and could help in better understanding of the expected behavior of the cache.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 18 Oct 2024 21:16:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/616/
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 19:14:34 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 2:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/615/
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 19:07:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5: Verified+1 -Code-Review
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 23:05:47 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/616/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 20:42:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21956/1//COMMIT_MSG
Commit Message:
PS1:
> Do you mind adding a unit test to cover this? That should help to catch re
I'm having trouble doing so, I can only access the high-level cache metrics in the testing code, not the segment-level metrics which we need to verify that the right shard is releasing the handle. I can only access the metrics that are part of CacheMetrics since that's what used in Cache::SetMetrics and the SLRUCache extends that implementation. The reference to the metrics is also a unique pointer so the only copy of the pointer can be in the cache (I can't store another copy in the testing code, so the only way to access the metrics would be through the slru_cache itself). Please let me know if you have any other ideas or if I'm missing something.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2024 20:16:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 4: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/625/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 23:49:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/627/ : FAILURE
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 17:56:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1:
> (1 comment)
Without going much into details, I see the description of this patch states that:
'... This patch fixes this bug so now the metrics should be accurate ...'
With that, I'd think you know of a scenario where user-facing metrics (i.e. CacheMetrics) were incorrect without this patch, but now they show correct numbers. Is it possible to programmatically implement a simplest scenario where it's be possible to see such a difference with and without the fix?
If going into more details a little bit, you could create a test and befriend it to a class that you are testing, and with that you can access any field and downcast anything to whatever you know it actually is. Maybe, then it be possible to capture the missing details, no?
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2024 05:04:53 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Reviewed-on: http://gerrit.cloudera.org:8080/21956
Tested-by: Alexey Serbin <al...@apache.org>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/util/slru_cache-test.cc
M src/kudu/util/slru_cache.cc
M src/kudu/util/slru_cache.h
3 files changed, 55 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 6
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc
File src/kudu/util/slru_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@302
PS3, Line 302: slru_cache_->metrics_.release()
> That was my reservation about this approach but I haven't found another way
The crucial point is: why to call 'release()' on the segment's metrics and remove it from under the segment? Would it suffice to have a raw pointer in the test, capturing it using 'get()' method instead?
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 00:21:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3: Verified+1
Unrelated test failures
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 19:06:49 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21956
to look at the new patch set (#5).
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
---
M src/kudu/util/slru_cache-test.cc
M src/kudu/util/slru_cache.cc
M src/kudu/util/slru_cache.h
3 files changed, 55 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/21956/5
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc
File src/kudu/util/slru_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@302
PS3, Line 302:
> The crucial point is: why to call 'release()' on the segment's metrics and
Yep that works, changed it.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:57:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 5:
> Patch Set 5: Verified-1
>
> Build Failed
>
> http://jenkins.kudu.apache.org/job/pre_commit/627/ : FAILURE
Unrelated test failures for the TSAN build in auto_leader_rebalancer-test and known flaky test org.apache.kudu.client.TestKuduTransaction on the Java side.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 5
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 18:00:30 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1:
> Patch Set 1:
>
> > (1 comment)
>
> Without going much into details, I see the description of this patch states that:
>
> '... This patch fixes this bug so now the metrics should be accurate ...'
>
> With that, I'd think you know of a scenario where user-facing metrics (i.e. CacheMetrics) were incorrect without this patch, but now they show correct numbers. Is it possible to programmatically implement a simplest scenario where it's be possible to see such a difference with and without the fix?
>
> If going into more details a little bit, you could create a test and befriend it to a class that you are testing, and with that you can access any field and downcast anything to whatever you know it actually is. Maybe, then it be possible to capture the missing details, no?
Yeh I went down that route, I just had to release the unique pointer to the metrics. I wanted to see if I was missing something and could test it out in a different way, but anyways thanks for the tips.
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 19:06:03 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21956
to look at the new patch set (#3).
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
---
M src/kudu/util/slru_cache-test.cc
M src/kudu/util/slru_cache.cc
M src/kudu/util/slru_cache.h
3 files changed, 54 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/21956/3
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21956
to look at the new patch set (#2).
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
---
M src/kudu/util/slru_cache-test.cc
M src/kudu/util/slru_cache.cc
M src/kudu/util/slru_cache.h
3 files changed, 52 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/21956/2
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 2
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/21956
to look at the new patch set (#4).
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
KUDU-613: Fix minor metrics related bug
When upserting in the protected segment, the state
of the handle isn't updated properly (the variable
in_protected_segment). This variable is used later
when releasing the handle to determine which segment's
metrics to update. This patch fixes this bug and adds
appropriate test coverage to verify the right metrics
are being updated.
Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
---
M src/kudu/util/slru_cache-test.cc
M src/kudu/util/slru_cache.cc
M src/kudu/util/slru_cache.h
3 files changed, 58 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/21956/4
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 4
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc
File src/kudu/util/slru_cache-test.cc:
PS3:
Thanks a lot for updating the scenario accordingly!
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@301
PS3, Line 301: static_cast
nit: consider using down_cast<> and asserting on the non-null of the result for safer down-casting of the pointers, to bullet-proof this part if anything changes in the metrics lineage in the future
http://gerrit.cloudera.org:8080/#/c/21956/3/src/kudu/util/slru_cache-test.cc@302
PS3, Line 302: slru_cache_->metrics_.release()
IIUC, this removes/nullifies the metrics from under the slru_cache_ instance. Is this really the desired behavior?
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 3
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 19:41:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-613: Fix minor metrics related bug
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21956 )
Change subject: KUDU-613: Fix minor metrics related bug
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/604/
--
To view, visit http://gerrit.cloudera.org:8080/21956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d2fd44169e1abe642c11e16e4b87aaa971b2d88
Gerrit-Change-Number: 21956
Gerrit-PatchSet: 1
Gerrit-Owner: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 18 Oct 2024 20:39:16 +0000
Gerrit-HasComments: No