wchevreuil commented on code in PR #7901:
URL: https://github.com/apache/hbase/pull/7901#discussion_r3148130295
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RowCache.java:
##########
@@ -63,8 +85,8 @@ <R> R execute(RowOperation<R> operation) throws IOException {
RowCache(Configuration conf) {
enabledByConf =
conf.getFloat(HConstants.ROW_CACHE_SIZE_KEY,
HConstants.ROW_CACHE_SIZE_DEFAULT) > 0;
- // TODO: implement row cache
- rowCacheStrategy = null;
+ // Currently we only support TinyLfu implementation
+ rowCacheStrategy = new
TinyLfuRowCacheStrategy(MemorySizeUtil.getRowCacheSize(conf));
Review Comment:
Should be made pluggable.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java:
##########
@@ -3435,6 +3473,15 @@ private void updateDeleteLatestVersionTimestamp(Cell
cell, Get get, int count, b
@Override
public void put(Put put) throws IOException {
TraceUtil.trace(() -> {
+ // Put with TTL is not allowed on tables with row cache enabled, because
cached rows cannot
+ // track TTL expiration
+ if (isRowCacheEnabled) {
+ if (put.getTTL() != Long.MAX_VALUE) {
+ throw new DoNotRetryIOException(
+ "Tables with row cache enabled do not allow setting TTL on Puts");
+ }
+ }
Review Comment:
Can't we simply implement TTL expiration within the RowCache.getRow logic?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RowCache.java:
##########
@@ -110,16 +229,67 @@ boolean tryGetFromCache(RowCacheKey key, Get get,
List<Cell> results) {
}
results.addAll(row.getCells());
- // TODO: implement update of metrics
return true;
}
- void populateCache(List<Cell> results, RowCacheKey key) {
- // TODO: implement with barrier to avoid cache read during mutation
- try {
- rowCacheStrategy.cacheRow(key, new RowCells(results));
- } catch (CloneNotSupportedException ignored) {
- // Not able to cache row cells, ignore
- }
+ void populateCache(HRegion region, List<Cell> results, RowCacheKey key) {
Review Comment:
nit: just call this method "cache".
Do we really need the regionLevelBarrierMap key type to be HRegion, or could
it be just the String encoded region name? That way, we don't need an extra
HRegion parameter in this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
