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 2019/04/30 23:11:17 UTC
[kudu-CR] [TTL cache] add periodic scrubbing thread
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13201
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread, invalidates expired entries, effectively
evicting them from the cache. With this patch, the TTLCache evicts
expired entries not only during the insertion of new ones if the cache
is at its capacity, but also upon the first detection of an entry's
expiry by the scrubbing thread.
Added corresponding tests as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 136 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/1
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#8).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/cache.h
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
3 files changed, 281 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/8
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#2).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon the first detection of an entry's expiry
by the scrubbing thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 145 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/2
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523
PS7, Line 523: // they must have been removed from the cache's lookup t
> I was not sure whether 64 is a good number of threads by default, but I wan
I guess my question was more along the lines of, "Why is the distinction between 16 and 64 important for slow tests / TSAN/ASAN?" Will this fail if we used 64 in all build modes? Or is this more just trying to avoid parameterization?
Coming back to this test a year from now, I'm not sure I'd follow the rationale for the separation. Could you add a comment explaining?
http://gerrit.cloudera.org:8080/#/c/13201/8/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/8/src/kudu/util/ttl_cache-test.cc@570
PS8, Line 570: const auto thread_idx = i;
nit: no longer need this
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 03 May 2019 00:41:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@429
PS5, Line 429: constexpr size_t cache_capacity = 512;
: const auto entry_ttl = MonoDelta::FromMilliseconds(250);
: const auto scrub_interval = MonoDelta::FromMilliseconds(
: entry_ttl.ToMilliseconds() / 2);
nit: use the GSG kNamingConvention?
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@447
PS5, Line 447: cache_capacity / 2
nit: pull this into some kNumKeysInserted or somesuch and use it here and below? Or just use the full capacity?
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@448
PS5, Line 448: 0$0
What's the point of the extra 0?
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@450
PS5, Line 450: std::any_of(std::begin(selected_keys), std::end(selected_keys),
: [&key](const string& e){ return e == key; })
nit: Would probably be more readable if we used a map?
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@460
PS5, Line 460: In this discrete setup, it's necessary to wait for an extra period
: // of the scrubbing thread to pass to avoid flaky results.
Isn't the period of the scrubbing thread determined by 'scrub_interval'?
This is probably not flaky now, but it's worth noting that we're also not taking into account the fact that it takes time to scrub (even if it is expected to be small).
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@483
PS5, Line 483:
: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: SleepFor(scrub_interval);
: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: ASSERT_EQ(cache_capacity, GetCacheUsage());
:
: SleepFor(entry_ttl);
:
: // Apart from the OS scheduler's anomalies, the scrubbing thread should run
: // at least once after at this point, and at least half of the entries
: // in the cache should have been invalidated by the time it ran.
: ASSERT_LE(cache_capacity, GetCacheEvictionsExpired());
: ASSERT_GE(cache_capacity / 2, GetCacheUsage());
What is this part of the test testing that isn't covered by the rest of the test?
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 20:04:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#4).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 147 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/4
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#5).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 148 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/5
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 6: Verified+1
unrelated flakes in tests:
* org.apache.kudu.client.TestTimeouts
* org.apache.kudu.spark.kudu.DefaultSourceTest
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 03:59:26 +0000
Gerrit-HasComments: No
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@464
PS2, Line 464: SleepFor(entry_ttl);
: SleepFor(entry_ttl);
How about just entry_ttl * 2 in one SleepFor?
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 30 Apr 2019 23:57:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13201/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/13201/6/src/kudu/util/ttl_cache.h@261
PS6, Line 261: // A reference to avoid passing 'this' into the capture of the cache
: // invalidation control below.
: const auto& max_entries_per_pass = max_scrubbed_entries_per_pass_num_;
What tangible difference does it make whether we capture 'this' or not? I can't see a reason why capturing 'this' is a bad idea, and it saves us three LOC over the current code (i.e. can remove the local ref and its comment).
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 03:25:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523
PS7, Line 523: // they must have been removed from the cache's lookup t
> I guess my question was more along the lines of, "Why is the distinction be
That's the general pattern you could see through the tests if you grep THREAD_SANITIZER -- tests with multiple threads run much longer in TSAN environment.
I think I'll better remove this ifdef instead of adding comments.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 03 May 2019 01:07:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#6).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 257 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/6
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@467
PS2, Line 467: ASSERT_EQ(cache_capacity / 2 - ARRAYSIZE(selected_keys),
> Done
Not fixed?
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@468
PS2, Line 468:
> Done
Not fixed?
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 16:20:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13201/1//COMMIT_MSG@10
PS1, Line 10: ,
> drop this
Done
http://gerrit.cloudera.org:8080/#/c/13201/1/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/13201/1/src/kudu/util/ttl_cache.h@264
PS1, Line 264: return valid_entry_count == 0;
> Not understanding the rationale here.
My bad: I forgot to comment on the reasoning. I added corresponding comment. I hope it's clearer now.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 30 Apr 2019 23:54:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@467
PS2, Line 467: // so those should not be deallocated yet.
> Not fixed?
Somehow it's not there. But I remember fixing those. Interesting.
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@468
PS2, Line 468:
> Not fixed?
Whoops. Now it's done.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 17:39:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#3).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 146 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/3
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13201/1//COMMIT_MSG@10
PS1, Line 10: ,
drop this
http://gerrit.cloudera.org:8080/#/c/13201/1/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/13201/1/src/kudu/util/ttl_cache.h@264
PS1, Line 264: return valid_entry_count == 0;
Not understanding the rationale here.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 30 Apr 2019 23:21:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523
PS7, Line 523: // they must have been removed from the cache's lookup t
> That's the general pattern you could see through the tests if you grep THRE
Right, I understand the rationale for having specific handling for TSAN, given how slow TSAN is. I think many of those were added because we found the test to be flaky or too slow otherwise. That didn't seem to be the case here, which is why I was confused.
Thanks for addressing it!
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 03 May 2019 01:11:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@483
PS5, Line 483:
: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: SleepFor(scrub_interval);
: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: ASSERT_EQ(cache_capacity, GetCacheUsage());
:
: SleepFor(entry_ttl);
:
: // Apart from the OS scheduler's anomalies, the scrubbing thread should run
: // at least once after at this point, and at least half of the entries
: // in the cache should have been invalidated by the time it ran.
: ASSERT_LE(cache_capacity, GetCacheEvictionsExpired());
: ASSERT_GE(cache_capacity / 2, GetCacheUsage());
> Gotcha, thanks for the explanation. It's much clearer now.
Done
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@442
PS7, Line 442:
> nit: indent.
Done
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523
PS7, Line 523:
> Just making sure I understand why this is important. With more threads, we'
I was not sure whether 64 is a good number of threads by default, but I wanted to have it covered as well, but paremeterizing on that didn't look like something's worth pursuing.
The difference in run-time is not big, though -- it's just in order of milliseconds. Probably, I'll better enable 64 threads for DEBUG/RELEASE builds and keep 16 for TSAN/ASAN.
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@544
PS7, Line 544:
> nit: isn't rand() enough to sufficiently differentiate the sleeps here? Is
Done
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@548
PS7, Line 548:
:
:
:
:
:
> nit: maybe this should go above the emplacement of the threads, in case thr
Done
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 23:53:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 7: Code-Review+1
(2 comments)
LGTM, just some nits
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@435
PS7, Line 435: AutoInvalidationOfExpiredEntries
nit: add a comment to describe the test.
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@442
PS7, Line 442:
nit: indent.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 20:46:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Reviewed-on: http://gerrit.cloudera.org:8080/13201
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/cache.h
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
3 files changed, 276 insertions(+), 13 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 18:23:09 +0000
Gerrit-HasComments: No
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@464
PS2, Line 464: // All the entries except for the one selected should be gone: the background
: // thread should get
> How about just entry_ttl * 2 in one SleepFor?
entry_ttl * 2 doesn't work as of now, but I think I can post a patch to allow for additions and multiplications for MonoDelta.
Here MonoDelta::FromMilliseconds(entry_ttl.ToMilliseconds() * 2)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@467
PS2, Line 467: ASSERT_EQ(cache_capacity / 2 - ARRAYSIZE(selected_keys),
> ones
Done
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@468
PS2, Line 468:
> You mean selected_handles here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 08:08:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 7: Code-Review+2
(4 comments)
LGTM, just minor nits that I don't feel particularly strongly about if you disagree.
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@483
PS5, Line 483: }
: // Now, once the handles for the selected entries are gone out of scope,
: // the cache should be empty.
: ASSERT_EQ(kHalfCacheCapacity, GetCacheEvictionsExpired());
: ASSERT_EQ(0, GetCacheUsage());
:
: // Make sure the scrubber doesn't conflate non-expired entries with expired
: // ones while scubbing the cache.
: for (auto i = 0; i < kHalfCacheCapacity; ++i) {
: cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: SleepFor(kEntryTtlThreeQuarters);
: for (auto i = 0; i < kHalfCacheCapacity; ++i) {
: cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: ASSERT_EQ(kCacheCapacity, GetCacheUsage());
: SleepFor(kEntryTtlThreeQuarters);
> What hasn't been covered yet: the scrubbing thread doesn't conflate expired
Gotcha, thanks for the explanation. It's much clearer now.
nit: it might be even clearer to separate this out into it's own test, so the
ASSERT_EQ(kHalfCacheCapacity, GetCacheEvictionsExpired());
wouldn't depend on that fact that we expired half the cache in the previous part of the test, and this functionality could be reasoned about in isolation.
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@523
PS7, Line 523: const auto kNumRunnerThreads = AllowSlowTests() ? 64 : 16;
Just making sure I understand why this is important. With more threads, we'll expect more handles in the cache active at any given time, and we might expect invalidation to avoid invalidating these "active" handles. Is that right?
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@544
PS7, Line 544: + thread_idx
nit: isn't rand() enough to sufficiently differentiate the sleeps here? Is this important? If not, we don't have to copy in the thread_idx
http://gerrit.cloudera.org:8080/#/c/13201/7/src/kudu/util/ttl_cache-test.cc@548
PS7, Line 548: SCOPED_CLEANUP({
: stop = true;
: for (auto& thread : threads) {
: thread.join();
: }
: });
nit: maybe this should go above the emplacement of the threads, in case thread creation fails for some reason? Though probably not important since I think all the threads will be cleaned up anyway.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 20:30:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@467
PS2, Line 467: // All the entries except for the one selected should be gone: the background
ones
http://gerrit.cloudera.org:8080/#/c/13201/2/src/kudu/util/ttl_cache-test.cc@468
PS2, Line 468: h_selected
You mean selected_handles here?
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 01 May 2019 05:23:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 7: Verified+1
Unrelated flakes in Java tests:
* org.apache.kudu.mapreduce.ITOutputFormatJob
* org.apache.kudu.client.TestRowErrors
* org.apache.kudu.client.TestKuduSession
* org.apache.kudu.client.TestAuthnTokenReacquireOpen
* org.apache.kudu.backup.TestKuduBackup
* org.apache.kudu.mapreduce.tools.ITImportParquetPreCheck
* org.apache.kudu.client.TestKuduScanner
* org.apache.kudu.backup.TestOptions
* org.apache.kudu.backup.TestKuduBackup
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 17:29:04 +0000
Gerrit-HasComments: No
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 03 May 2019 00:31:51 +0000
Gerrit-HasComments: No
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@429
PS5, Line 429: constexpr size_t cache_capacity = 512;
: const auto entry_ttl = MonoDelta::FromMilliseconds(250);
: const auto scrub_interval = MonoDelta::FromMilliseconds(
: entry_ttl.ToMilliseconds() / 2);
> nit: use the GSG kNamingConvention?
Done
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@447
PS5, Line 447: cache_capacity / 2
> nit: pull this into some kNumKeysInserted or somesuch and use it here and b
Done
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@448
PS5, Line 448: 0$0
> What's the point of the extra 0?
More entries are inserted later on using similar pattern for the key. The idea is to distinguish between those, at least in VLOG() print-outs.
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@450
PS5, Line 450: std::any_of(std::begin(selected_keys), std::end(selected_keys),
: [&key](const string& e){ return e == key; })
> nit: Would probably be more readable if we used a map?
You meant std::set? Done.
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@460
PS5, Line 460: In this discrete setup, it's necessary to wait for an extra period
: // of the scrubbing thread to pass to avoid flaky results.
> Isn't the period of the scrubbing thread determined by 'scrub_interval'?
Right, but it's necessary to wait for the entries to expire. Given that scrub_interval == entry_ttl / 2, that gives a good margin.
Right, this is not flaky now: even with TSAN builds and --stress-cpu-threads=16. To make it more stable, I changed scrub_interval to be 1/8 of the entry_ttl.
I'm not sure it's possible to write 100% bullet-proof test that depends on a relative timing of independent threads, and I'm sure it's not possible in the presence of OS scheduler anomalies. However, this scenario gives good coverage and is stable enough. Also, I replaced those pauses with ASSERT_EVENTUALLY in most places where possible.
http://gerrit.cloudera.org:8080/#/c/13201/5/src/kudu/util/ttl_cache-test.cc@483
PS5, Line 483: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("1$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: SleepFor(scrub_interval);
: for (auto i = 0; i < cache_capacity / 2; ++i) {
: cache.Put(Substitute("2$0", i), unique_ptr<TestValue>(new TestValue), 1);
: }
: ASSERT_EQ(cache_capacity, GetCacheUsage());
:
: SleepFor(entry_ttl);
:
: // Apart from the OS scheduler's anomalies, the scrubbing thread should run
: // at least once after at this point, and at least half of the entries
: // in the cache should have been invalidated by the time it ran.
: ASSERT_LE(cache_capacity, GetCacheEvictionsExpired());
: ASSERT_GE(cache_capacity / 2, GetCacheUsage());
: }
> What is this part of the test testing that isn't covered by the rest of the
What hasn't been covered yet: the scrubbing thread doesn't conflate expired entries with non-expired ones. It seems the comment and the assertions were misleading: I updated those.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 01:37:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#9).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/cache.h
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
3 files changed, 276 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/9
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 9: Verified+1
unrelated flakes in Java tests:
* org.apache.kudu.backup.TestKuduBackup
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 03 May 2019 01:37:21 +0000
Gerrit-HasComments: No
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13201
to look at the new patch set (#7).
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
[TTL cache] add periodic scrubbing thread
This patch adds an optional periodic scrubbing thread for the TTLCache.
If enabled, the thread invalidates expired entries, effectively evicting
them from the cache. With this patch, the TTLCache evicts expired
entries not only during the insertion of new ones if the cache is at its
capacity, but also upon detection of an entry's expiry by the scrubbing
thread.
Added corresponding test coverage as well.
Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
---
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
2 files changed, 253 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/13201/7
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [TTL cache] add periodic scrubbing thread
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13201 )
Change subject: [TTL cache] add periodic scrubbing thread
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13201/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/13201/6/src/kudu/util/ttl_cache.h@261
PS6, Line 261: // A reference to avoid passing 'this' into the capture of the cache
: // invalidation control below.
: const auto& max_entries_per_pass = max_scrubbed_entries_per_pass_num_;
> What tangible difference does it make whether we capture 'this' or not? I c
It's a good question. I'm not sure there is any tangible difference.
At some point I tried to access the max_scrubbed_entries_per_pass_num_ member from within the lambda, but compiler reported an error, so I created the const reference variable.
I'll see how it works with capturing 'this' or just 'this->max_scrubbed_entries_per_pass_num_'.
--
To view, visit http://gerrit.cloudera.org:8080/13201
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3bbbcf6211930a5501a03d6a47f3d911118946c
Gerrit-Change-Number: 13201
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 04:03:39 +0000
Gerrit-HasComments: Yes