You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2021/11/13 01:23:51 UTC
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18024
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
[client] fix MetaCacheEntry::Contains()
It seems that while working on a50091e2d, I went too far with
back-porting changes from the revamped code and used the unabridged
MetaCacheEntry::Contains() implementation from the new approach. That
turned to be problematic if still using the legacy way of comparing
partition keys for tables using only hash partitioning.
This patch returns the legacy implementation of the
MetaCacheEntry::Contains() method back and adds a couple of stress
test scenarios for the C++ client's metacache. Before reverting to
the legacy implementation of the MetaCacheEntry::Contains() method,
both Perf and PerfSynthetic test scenarios were failing because
KuduDeleteIgnore operations would timeout since MetaCache was endlessly
fetching information on particular PartitionKeys again and again when
MetaCacheEntry::Contains() returned 'false' instead of returning 'true'.
This is a follow-up to a50091e2d4509feac2f29128107102ec52fcb7b0.
Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
---
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/integration-tests/client-stress-test.cc
4 files changed, 168 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/18024/1
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3: Code-Review+1
LGTM, thanks for fixing this.
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Nov 2021 08:17:09 +0000
Gerrit-HasComments: No
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@337
PS3, Line 337: #if !defined(THREAD_SANITIZER) && !defined(ADDRESS_SANITIZER)
> I think there are examples for both approaches in our codebase. Anyway, I d
All right, thanks! Then I'll post a small patch to build the client-test-test binary only in case of non-ASAN/non-TSAN builds.
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Nov 2021 18:55:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@337
PS3, Line 337: #if !defined(THREAD_SANITIZER) && !defined(ADDRESS_SANITIZER)
> Would it make sense to change this so that we call GTEST_SKIP() if either o
I'm not sure it makes a lot of sense to always call GTEST_SKIP in ASAN and TSAN builds: why to compile and link all this code at all those builds then?
Probably, it makes sense to skip all the scenarios in this file under ASAN/TSAN builds. I think I can post a separate changelist to remove this file from the CMakeLists.txt in case of TSAN/ASAN builds (we do something similar with client_examples-test.sh under src/kudu/client).
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@342
PS3, Line 342: // Preliminary work to do before running the benchmark scenarios. Could be
> Does this comment still make sense? Seems like all the setup is in SetUp(),
Done
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@375
PS3, Line 375: {
> nit: why this extra block?
Done
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@445
PS3, Line 445: large
> nit: extra word
Done
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Nov 2021 15:40:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 2: Code-Review+2
(3 comments)
LGTM, though it's also worth getting a look from Mahesh.
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc@379
PS2, Line 379: unique_ptr<KuduDeleteIgnore> op(table_->NewDeleteIgnore());
Clever!
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc@441
PS2, Line 441: vector<unique_ptr<KuduDeleteIgnore>> ops;
: ops.reserve(kNumRows);
: {
: for (int32_t idx = 0; idx < kNumRows; ++idx) {
: unique_ptr<KuduDeleteIgnore> op(table_->NewDeleteIgnore());
: ASSERT_OK(op->mutable_row()->SetInt64(0, idx));
: ASSERT_OK(op->mutable_row()->SetString(1,
: std::to_string(idx) + string(1, static_cast<char>(idx % 128))));
: ops.emplace_back(std::move(op));
: }
: }
nit: consider an anonymous function to share across these tests
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc@462
PS2, Line 462: client::sp::shared_ptr<KuduSession> session(client_->NewSession());
: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH));
: ASSERT_OK(session->SetMutationBufferSpace(64 * 1024 * 1024));
nit: are we intentionally timing these too? if not, consider moving it out of the timed block
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 13 Nov 2021 03:24:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18024
to look at the new patch set (#3).
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
[client] fix MetaCacheEntry::Contains()
It seems that while working on a50091e2d, I went too far with
back-porting changes from the revamped code and used the unabridged
MetaCacheEntry::Contains() implementation from the new approach. That
turned to be problematic if still using the legacy way of comparing
partition keys for tables using only hash partitioning.
This patch returns the legacy implementation of the
MetaCacheEntry::Contains() method back and adds a couple of stress
test scenarios for the C++ client's metacache. Before reverting to
the legacy implementation of the MetaCacheEntry::Contains() method,
both Perf and PerfSynthetic test scenarios were failing because
KuduDeleteIgnore operations would timeout since MetaCache was endlessly
fetching information on particular PartitionKeys again and again when
MetaCacheEntry::Contains() returned 'false' instead of returning 'true'.
This is a follow-up to a50091e2d4509feac2f29128107102ec52fcb7b0.
Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
---
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/integration-tests/client-stress-test.cc
4 files changed, 159 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/18024/3
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3: Verified+1
unrelated test failure in TxnOpDispatcherITest.TxnWriteWhileReplicaDeleted
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 13 Nov 2021 17:25:50 +0000
Gerrit-HasComments: No
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
[client] fix MetaCacheEntry::Contains()
It seems that while working on a50091e2d, I went too far with
back-porting changes from the revamped code and used the unabridged
MetaCacheEntry::Contains() implementation from the new approach. That
turned to be problematic if still using the legacy way of comparing
partition keys for tables using only hash partitioning.
This patch returns the legacy implementation of the
MetaCacheEntry::Contains() method back and adds a couple of stress
test scenarios for the C++ client's metacache. Before reverting to
the legacy implementation of the MetaCacheEntry::Contains() method,
both Perf and PerfSynthetic test scenarios were failing because
KuduDeleteIgnore operations would timeout since MetaCache was endlessly
fetching information on particular PartitionKeys again and again when
MetaCacheEntry::Contains() returned 'false' instead of returning 'true'.
This is a follow-up to a50091e2d4509feac2f29128107102ec52fcb7b0.
Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Reviewed-on: http://gerrit.cloudera.org:8080/18024
Tested-by: Kudu Jenkins
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/integration-tests/client-stress-test.cc
4 files changed, 154 insertions(+), 15 deletions(-)
Approvals:
Kudu Jenkins: Verified
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Tidy Bot, Attila Bukor, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18024
to look at the new patch set (#4).
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
[client] fix MetaCacheEntry::Contains()
It seems that while working on a50091e2d, I went too far with
back-porting changes from the revamped code and used the unabridged
MetaCacheEntry::Contains() implementation from the new approach. That
turned to be problematic if still using the legacy way of comparing
partition keys for tables using only hash partitioning.
This patch returns the legacy implementation of the
MetaCacheEntry::Contains() method back and adds a couple of stress
test scenarios for the C++ client's metacache. Before reverting to
the legacy implementation of the MetaCacheEntry::Contains() method,
both Perf and PerfSynthetic test scenarios were failing because
KuduDeleteIgnore operations would timeout since MetaCache was endlessly
fetching information on particular PartitionKeys again and again when
MetaCacheEntry::Contains() returned 'false' instead of returning 'true'.
This is a follow-up to a50091e2d4509feac2f29128107102ec52fcb7b0.
Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
---
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/integration-tests/client-stress-test.cc
4 files changed, 154 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/18024/4
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@337
PS3, Line 337: #if !defined(THREAD_SANITIZER) && !defined(ADDRESS_SANITIZER)
> I'm not sure it makes a lot of sense to always call GTEST_SKIP in ASAN and
I think there are examples for both approaches in our codebase. Anyway, I don't feel strongly about it, just thought it might be more developer friendly to see these tests are skipped.
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Nov 2021 18:26:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc@441
PS2, Line 441: vector<unique_ptr<KuduDeleteIgnore>> ops;
: ops.reserve(kNumRows);
: {
: for (int32_t idx = 0; idx < kNumRows; ++idx) {
: unique_ptr<KuduDeleteIgnore> op(table_->NewDeleteIgnore());
: ASSERT_OK(op->mutable_row()->SetInt64(0, idx));
: ASSERT_OK(op->mutable_row()->SetString(1,
: std::to_string(idx) + string(1, static_cast<char>(idx % 128))));
: ops.emplace_back(std::move(op));
: }
: }
> nit: consider an anonymous function to share across these tests
Ah, good point!
Done.
http://gerrit.cloudera.org:8080/#/c/18024/2/src/kudu/integration-tests/client-stress-test.cc@462
PS2, Line 462: client::sp::shared_ptr<KuduSession> session(client_->NewSession());
: ASSERT_OK(session->SetFlushMode(KuduSession::MANUAL_FLUSH));
: ASSERT_OK(session->SetMutationBufferSpace(64 * 1024 * 1024));
> nit: are we intentionally timing these too? if not, consider moving it out
Done
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 13 Nov 2021 06:35:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18024
to look at the new patch set (#2).
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
[client] fix MetaCacheEntry::Contains()
It seems that while working on a50091e2d, I went too far with
back-porting changes from the revamped code and used the unabridged
MetaCacheEntry::Contains() implementation from the new approach. That
turned to be problematic if still using the legacy way of comparing
partition keys for tables using only hash partitioning.
This patch returns the legacy implementation of the
MetaCacheEntry::Contains() method back and adds a couple of stress
test scenarios for the C++ client's metacache. Before reverting to
the legacy implementation of the MetaCacheEntry::Contains() method,
both Perf and PerfSynthetic test scenarios were failing because
KuduDeleteIgnore operations would timeout since MetaCache was endlessly
fetching information on particular PartitionKeys again and again when
MetaCacheEntry::Contains() returned 'false' instead of returning 'true'.
This is a follow-up to a50091e2d4509feac2f29128107102ec52fcb7b0.
Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
---
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/integration-tests/client-stress-test.cc
4 files changed, 166 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/18024/2
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 13 Nov 2021 19:46:49 +0000
Gerrit-HasComments: No
[kudu-CR] [client] fix MetaCacheEntry::Contains()
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18024 )
Change subject: [client] fix MetaCacheEntry::Contains()
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc
File src/kudu/integration-tests/client-stress-test.cc:
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@337
PS3, Line 337: #if !defined(THREAD_SANITIZER) && !defined(ADDRESS_SANITIZER)
Would it make sense to change this so that we call GTEST_SKIP() if either of these is defined instead of not even having these tests in those builds?
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@342
PS3, Line 342: // Preliminary work to do before running the benchmark scenarios. Could be
Does this comment still make sense? Seems like all the setup is in SetUp(), except for GenerateOperations.
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@375
PS3, Line 375: {
nit: why this extra block?
http://gerrit.cloudera.org:8080/#/c/18024/3/src/kudu/integration-tests/client-stress-test.cc@445
PS3, Line 445: large
nit: extra word
--
To view, visit http://gerrit.cloudera.org:8080/18024
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f2deeefbf2e3cedc7654b5d977382501db14b4e
Gerrit-Change-Number: 18024
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 15 Nov 2021 13:00:31 +0000
Gerrit-HasComments: Yes