You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Joe McDonnell (Code Review)" <ge...@cloudera.org> on 2020/02/07 22:35:42 UTC
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Joe McDonnell has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15179
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free need to treat the handle as an object
(calling constructors/destructors) rather than a treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 365 insertions(+), 282 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/2
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 10: Code-Review+2
Rebased, carrying +2
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 01:29:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free need to treat the handle as an object
(calling constructors/destructors) rather than a treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 365 insertions(+), 282 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/3
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 7:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@23
PS7, Line 23: need
> nit: "needs"
Done
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@24
PS7, Line 24: a
> nit: delete
Done
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@30
PS7, Line 30: point to
> nit: point to what exactly?
Reworked this a bit to describe how the contiguous layout would work.
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@34
PS7, Line 34: HandleBase(uint8_t* kv_ptr, const Slice& key, int32_t hash, int val_len, int charge) {
> Might be useful to have a comment, eg. "kv_ptr points to memory allocated t
Added a comment to try to explain what is expected for kv_ptr.
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@46
PS7, Line 46:
> formatting
Done
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@68
PS7, Line 68: friend class HandleTable;
> nit: formatting
Done
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@215
PS7, Line 215: mem_tracker_ = kudu::MemTracker::FindOrCreateGlobalTracker(
> Do you know how, if at all, we integration this with our own mem tracking,
Right now, I checked and it doesn't integrate. This is actually a good thing, because we are accounting the whole cache usage which is larger than memory. My plan for this is that the mem tracker will change to an Impala version, and it will track only the metadata's memory usage (not on disk cache usage).
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc
File be/src/util/cache/cache.cc:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a79
PS7, Line 79:
> I think this comment is still true and useful, wrt HandleBase::kv_data_ptr_
Brought this back for kv_data_ptr_.
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a554
PS7, Line 554:
> This appears to have been dropped. Is it no longer the case?
I think the -1 comes from the one byte that is already part of sizeof(RLHandle):
uint8_t kv_data[1];
Since we no longer have that entry, I removed the -1.
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a560
PS7, Line 560:
> I think this comment may also still be useful?
Added this back to RLCacheShard::Allocate().
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@83
PS7, Line 83: A single shard of a recency list eviction policy
> nit: revise to "A single shared of a cache that uses a recency list based e
Done
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@84
PS7, Line 84: template<Cache::EvictionPolicy policy>
: class RLCacheShard : public CacheShard
> perhaps this can be addressed later, but it looks like this implementation
Yes, I think this cache implementation will only ever do FIFO/LRU. Other algorithms will implement the CacheShard interface, but they are unlikely to share code with this. I think this will not compile with something other than FIFO/LRU due to RL_UpdateAfterLookup only have implementations for FIFO/LRU, but I also added a static_assert that RLCacheShard is only used for FIFO/LRU.
The next patch adds LIRS, so it will give a chance to fix anything that ends up looking wrong.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2020 23:44:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5322/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 25 Feb 2020 00:31:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5264/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Feb 2020 02:34:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5353/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 02:16:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Feb 2020 03:12:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15179/5/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/5/be/src/util/cache/cache-internal.h@269
PS5, Line 269: HandleType* h = reinterpret_cast<HandleType*>(pending_handle);
: delete h;
> I ran ASAN, and it is unhappy about the asymmetry between using a new to al
Reworked this code to make things symmetric. I also pushed the Allocate/Free calls down to the shards to eliminate a template parameter on the ShardedCache.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Feb 2020 01:48:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free needs to treat the handle as an object
(calling constructors/destructors) rather than treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Testing:
- Release core run with a data cache enabled
- ASAN core
- The cache-test backend test continues to pass
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Reviewed-on: http://gerrit.cloudera.org:8080/15179
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 426 insertions(+), 286 deletions(-)
Approvals:
Joe McDonnell: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 11
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5416/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 01:30:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 05:57:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5336/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Feb 2020 22:23:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 7:
(1 comment)
Rebased
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@215
PS7, Line 215: mem_tracker_ = kudu::MemTracker::FindOrCreateGlobalTracker(
> Maybe add a TODO for this?
Added a TODO
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 01:29:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5403/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 01:30:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5167/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 07 Feb 2020 23:20:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Sahil Takiar, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15179
to look at the new patch set (#10).
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free needs to treat the handle as an object
(calling constructors/destructors) rather than treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Testing:
- Release core run with a data cache enabled
- ASAN core
- The cache-test backend test continues to pass
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 426 insertions(+), 286 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/10
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 10
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Sahil Takiar, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15179
to look at the new patch set (#8).
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free needs to treat the handle as an object
(calling constructors/destructors) rather than treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Testing:
- Release core run with a data cache enabled
- ASAN core
- The cache-test backend test continues to pass
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 424 insertions(+), 286 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/8
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 8
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@34
PS7, Line 34: HandleBase(uint8_t* kv_ptr, const Slice& key, int32_t hash, int val_len, int charge) {
Might be useful to have a comment, eg. "kv_ptr points to memory allocated to fit..."
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@46
PS7, Line 46:
formatting
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@215
PS7, Line 215: mem_tracker_ = kudu::MemTracker::FindOrCreateGlobalTracker(
Do you know how, if at all, we integration this with our own mem tracking, eg. ExecEnv::process_mem_tracker or the /memz webui page?
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc
File be/src/util/cache/cache.cc:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a79
PS7, Line 79:
I think this comment is still true and useful, wrt HandleBase::kv_data_ptr_?
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a554
PS7, Line 554:
This appears to have been dropped. Is it no longer the case?
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a560
PS7, Line 560:
I think this comment may also still be useful?
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2020 18:56:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free need to treat the handle as an object
(calling constructors/destructors) rather than a treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 407 insertions(+), 286 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/6
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 6
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 9: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@215
PS7, Line 215: // HandleBase itself. The number of shards is currently a power of 2 so that it can
> Right now, I checked and it doesn't integrate. This is actually a good thin
Maybe add a TODO for this?
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Feb 2020 00:13:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15179
to look at the new patch set (#4).
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
IMPALA-8690 (prep 3): Factor out common code for cache implementations
The existing cache implementation handles LRU/FIFO eviction algorithms,
but it also implements several components that are useful for other
cache implementations. Specifically, there is a simple hash table
(HandleTable) and a simple sharding implementation (ShardedCache).
These can be reused across other eviction algorithms by making them
generic.
This pulls them out of be/src/util/cache/cache.cc and into a new
be/src/util/cache/cache-internal.h file. To make the HandleTable
generic, this introduces a HandleBase class that contains common
code between the implementations (such as the key, the value,
the hash, etc). The HandleTable works on this base class, and the
RLHandle now derives from HandleBase.
To support this, Allocate/Free need to treat the handle as an object
(calling constructors/destructors) rather than a treating it like
a chunk of memory (or simple struct).
ShardedCache is made generic by having cache implementations derive
from a base CacheShard class that defines the appropriate methods
needed by the sharding class. This is purely an interface, and the
base class defines no functions. The existing CacheShard is renamed
to RLCachedShard and derives from this class.
Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
---
A be/src/util/cache/cache-internal.h
M be/src/util/cache/cache.cc
M bin/rat_exclude_files.txt
3 files changed, 365 insertions(+), 282 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/15179/4
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5172/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 3
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Feb 2020 00:33:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5177/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 08 Feb 2020 02:15:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15179/5/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/5/be/src/util/cache/cache-internal.h@269
PS5, Line 269: HandleType* h = reinterpret_cast<HandleType*>(pending_handle);
: delete h;
I ran ASAN, and it is unhappy about the asymmetry between using a new to allocate an array and then using a regular delete on this. I'm fixing it to make it symmetric (i.e. call destructor and then call delete []).
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 5
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Feb 2020 19:01:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 9
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 26 Feb 2020 05:58:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8690 (prep 3): Factor out common code for cache implementations
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations
......................................................................
Patch Set 7:
(6 comments)
mostly nits, still trying to understand the data-cache code
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@23
PS7, Line 23: need
nit: "needs"
http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@24
PS7, Line 24: a
nit: delete
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h
File be/src/util/cache/cache-internal.h:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@30
PS7, Line 30: point to
nit: point to what exactly?
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@68
PS7, Line 68: friend class HandleTable;
nit: formatting
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc
File be/src/util/cache/cache.cc:
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@83
PS7, Line 83: A single shard of a recency list eviction policy
nit: revise to "A single shared of a cache that uses a recency list based eviction policy"
http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@84
PS7, Line 84: template<Cache::EvictionPolicy policy>
: class RLCacheShard : public CacheShard
perhaps this can be addressed later, but it looks like this implementation of CacheShard is specific to recency list eviction algorithms, yet the template suggests it can be applied to any EvictionPolicy. not really an issue since all the eviction algorithms are RL specific, but perhaps we should consider re-factoring this at some point. maybe add a Cache::RLEvictionPolicy
--
To view, visit http://gerrit.cloudera.org:8080/15179
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e
Gerrit-Change-Number: 15179
Gerrit-PatchSet: 7
Gerrit-Owner: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Feb 2020 21:22:44 +0000
Gerrit-HasComments: Yes