You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/12/14 19:17:33 UTC
[GitHub] [ozone] bharatviswa504 opened a new pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
bharatviswa504 opened a new pull request #1699:
URL: https://github.com/apache/ozone/pull/1699
## What changes were proposed in this pull request?
1. Implement TableCache based on cache type FullTableCache and PartialTable Cache.
2. Fix cleanup eviction logic, where epoch entires are not getting removed.
3. Fix issue of race condition between request processing and cleanup thread in FullTableCache. This is solved using cache locks.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4583
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
Existing TestTableCache tests and added a test.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#issuecomment-750444554
Thanks @bharatviswa504.
PR LGTM. +1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#discussion_r547557289
##########
File path: hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/cache/TestTableCache.java
##########
@@ -380,6 +383,33 @@ public void testPartialTableCacheParallel() throws Exception {
}
+ @Test
+ public void testTableCache() {
+
+ // In non-HA epoch entries might be out of order.
+ // Scenario is like create vol, set vol, set vol, delete vol
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(0)), 0));
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(1)), 1));
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(2)), 3));
+
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.absent(), 2));
+
+ List<Long> epochs = new ArrayList<>();
+ epochs.add(0L);
+ epochs.add(1L);
+ epochs.add(2L);
+ epochs.add(3L);
+
Review comment:
Added a new test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 merged pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #1699:
URL: https://github.com/apache/ozone/pull/1699
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#issuecomment-753223567
Thank You @hanishakoneru for the review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#issuecomment-749788712
Thanks @bharatviswa504 for working on this.
PR LGTM overall. Posted some comments and suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#issuecomment-749834171
Thank You @hanishakoneru for the review. Addressed review comments in the latest commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on a change in pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#discussion_r546957392
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java
##########
@@ -307,12 +307,11 @@ protected ObjectName getStatMBeanName() {
valueType);
}
- @Override
Review comment:
Any reason for removing the Override flag?
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStore.java
##########
@@ -61,14 +60,9 @@
<KEY, VALUE> Table<KEY, VALUE> getTable(String name,
Class<KEY> keyType, Class<VALUE> valueType) throws IOException;
- /**
- * Gets an existing TableStore with implicit key/value conversion and
- * with specified cleanup policy for cache.
- * @throws IOException
- */
<KEY, VALUE> Table<KEY, VALUE> getTable(String name,
Review comment:
Can we add new JavaDoc here.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/PartialTableCache.java
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.hadoop.hdds.utils.db.cache;
+
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.NavigableSet;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListSet;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import org.apache.hadoop.hdds.annotation.InterfaceAudience.Private;
+import org.apache.hadoop.hdds.annotation.InterfaceStability.Evolving;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Cache implementation for the table. Partial Table cache, where the DB state
+ * and cache state will not be same. Partial table cache holds entries until
+ * flush to DB happens.
+ */
+@Private
+@Evolving
+public class PartialTableCache<CACHEKEY extends CacheKey,
+ CACHEVALUE extends CacheValue> implements TableCache<CACHEKEY, CACHEVALUE> {
+
+ public static final Logger LOG =
+ LoggerFactory.getLogger(PartialTableCache.class);
+
+ private final Map<CACHEKEY, CACHEVALUE> cache;
+ private final NavigableSet<EpochEntry<CACHEKEY>> epochEntries;
+ private ExecutorService executorService;
+
+
+ public PartialTableCache() {
+ // We use concurrent Hash map for O(1) lookup for get API.
+ // During list operation for partial cache we anyway merge between DB and
+ // cache state. So entries in cache does not need to be in sorted order.
+
+ // And as concurrentHashMap computeIfPresent which is used by cleanup is
+ // atomic operation, and ozone level locks like bucket/volume locks
+ // protect updating same key, here it is not required to hold cache
+ // level locks during update/cleanup operation.
+
+ // 1. During update, it is caller responsibility to hold volume/bucket
+ // locks.
+ // 2. During cleanup which removes entry, while request is updating cache
+ // that should be guarded by concurrentHashMap guaranty.
+ cache = new ConcurrentHashMap<>();
+
+ epochEntries = new ConcurrentSkipListSet<>();
+ // Created a singleThreadExecutor, so one cleanup will be running at a
+ // time.
+ ThreadFactory build = new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat("PartialTableCache Cleanup Thread - %d").build();
+ executorService = Executors.newSingleThreadExecutor(build);
+ }
+
+ @Override
+ public CACHEVALUE get(CACHEKEY cachekey) {
+ return cache.get(cachekey);
+ }
+
+ @Override
+ public void loadInitial(CACHEKEY cacheKey, CACHEVALUE cacheValue) {
+ // Do nothing for full table cache.
Review comment:
Typo: Partial table cache
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/PartialTableCache.java
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.hadoop.hdds.utils.db.cache;
+
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.NavigableSet;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListSet;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import org.apache.hadoop.hdds.annotation.InterfaceAudience.Private;
+import org.apache.hadoop.hdds.annotation.InterfaceStability.Evolving;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Cache implementation for the table. Partial Table cache, where the DB state
+ * and cache state will not be same. Partial table cache holds entries until
+ * flush to DB happens.
+ */
+@Private
+@Evolving
+public class PartialTableCache<CACHEKEY extends CacheKey,
+ CACHEVALUE extends CacheValue> implements TableCache<CACHEKEY, CACHEVALUE> {
+
+ public static final Logger LOG =
+ LoggerFactory.getLogger(PartialTableCache.class);
+
+ private final Map<CACHEKEY, CACHEVALUE> cache;
+ private final NavigableSet<EpochEntry<CACHEKEY>> epochEntries;
+ private ExecutorService executorService;
+
+
+ public PartialTableCache() {
+ // We use concurrent Hash map for O(1) lookup for get API.
+ // During list operation for partial cache we anyway merge between DB and
+ // cache state. So entries in cache does not need to be in sorted order.
+
+ // And as concurrentHashMap computeIfPresent which is used by cleanup is
+ // atomic operation, and ozone level locks like bucket/volume locks
+ // protect updating same key, here it is not required to hold cache
+ // level locks during update/cleanup operation.
+
+ // 1. During update, it is caller responsibility to hold volume/bucket
+ // locks.
+ // 2. During cleanup which removes entry, while request is updating cache
+ // that should be guarded by concurrentHashMap guaranty.
+ cache = new ConcurrentHashMap<>();
+
+ epochEntries = new ConcurrentSkipListSet<>();
+ // Created a singleThreadExecutor, so one cleanup will be running at a
+ // time.
+ ThreadFactory build = new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat("PartialTableCache Cleanup Thread - %d").build();
+ executorService = Executors.newSingleThreadExecutor(build);
+ }
+
+ @Override
+ public CACHEVALUE get(CACHEKEY cachekey) {
+ return cache.get(cachekey);
+ }
+
+ @Override
+ public void loadInitial(CACHEKEY cacheKey, CACHEVALUE cacheValue) {
+ // Do nothing for full table cache.
+ }
+
+ @Override
+ public void put(CACHEKEY cacheKey, CACHEVALUE value) {
+ cache.put(cacheKey, value);
+ epochEntries.add(new EpochEntry<>(value.getEpoch(), cacheKey));
+ }
+
+ public void cleanup(List<Long> epochs) {
+ executorService.execute(() -> evictCache(epochs));
+ }
+
+ @Override
+ public int size() {
+ return cache.size();
+ }
+
+ @Override
+ public Iterator<Map.Entry<CACHEKEY, CACHEVALUE>> iterator() {
+ return cache.entrySet().iterator();
+ }
+
+ @VisibleForTesting
+ public void evictCache(List<Long> epochs) {
+ EpochEntry<CACHEKEY> currentEntry;
+ final AtomicBoolean removed = new AtomicBoolean();
+ CACHEKEY cachekey;
+ long lastEpoch = epochs.get(epochs.size() - 1);
+ for (Iterator<EpochEntry<CACHEKEY>> iterator = epochEntries.iterator();
+ iterator.hasNext();) {
+ currentEntry = iterator.next();
+ cachekey = currentEntry.getCachekey();
+ long currentEpoch = currentEntry.getEpoch();
+
+ // As ConcurrentHashMap computeIfPresent is atomic, there is no race
+ // condition between cache cleanup and requests updating same cache entry.
+
+ cache.computeIfPresent(cachekey, ((k, v) -> {
+ if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("CacheKey {} with epoch {} is removed from cache",
+ k.getCacheKey(), currentEpoch);
+ }
+ iterator.remove();
+ removed.set(true);
+ return null;
+ }
+ return v;
+ }));
+
+ // If currentEntry epoch is greater than last epoch provided, we have
+ // deleted all entries less than specified epoch. So, we can break.
+ if (currentEpoch > lastEpoch) {
+ break;
+ }
+
+ // When epoch entry is not removed, this might be a override entry in
+ // cache. Clean that epoch entry.
+ if (!removed.get()) {
Review comment:
Same suggestion as in FullTableCache for removed usage.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/TypedTable.java
##########
@@ -61,8 +62,7 @@
/**
* Create an TypedTable from the raw table.
- * Default cleanup policy used for the table is
- * {@link CacheCleanupPolicy#MANUAL}.
+ * Default cache type for the table is {@link CacheType#FullCache}.
Review comment:
JavaDoc says default cache type is FullCache but the calling parameter is set to PartialCache.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -125,43 +136,44 @@ protected void evictCache(List<Long> epochs) {
currentEntry = iterator.next();
cachekey = currentEntry.getCachekey();
long currentEpoch = currentEntry.getEpoch();
- CacheValue cacheValue = cache.computeIfPresent(cachekey, ((k, v) -> {
- if (cleanupPolicy == CacheCleanupPolicy.MANUAL) {
- if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- iterator.remove();
- removed.set(true);
- return null;
- }
- } else if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- // Remove only entries which are marked for delete.
+
+ // Acquire lock to avoid race between cleanup and add to cache entry by
+ // client requests.
+ try {
+ lock.writeLock().lock();
+ cache.computeIfPresent(cachekey, ((k, v) -> {
if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())
&& v.getCacheValue() == null) {
Review comment:
Can we add the comment back - `// Remove only entries which are marked for delete.` and elaborate more that only the epoch entry corresponding to the current CacheValue will be removed here.
##########
File path: hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/db/cache/TestTableCache.java
##########
@@ -380,6 +383,33 @@ public void testPartialTableCacheParallel() throws Exception {
}
+ @Test
+ public void testTableCache() {
+
+ // In non-HA epoch entries might be out of order.
+ // Scenario is like create vol, set vol, set vol, delete vol
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(0)), 0));
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(1)), 1));
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.of(Long.toString(2)), 3));
+
+ tableCache.put(new CacheKey<>(Long.toString(0)),
+ new CacheValue<>(Optional.absent(), 2));
+
+ List<Long> epochs = new ArrayList<>();
+ epochs.add(0L);
+ epochs.add(1L);
+ epochs.add(2L);
+ epochs.add(3L);
+
Review comment:
Can we add the case where evictCache is called with an missing entry from consecutive list of epochs.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/PartialTableCache.java
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.hadoop.hdds.utils.db.cache;
+
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.NavigableSet;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListSet;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.util.concurrent.ThreadFactoryBuilder;
+import org.apache.hadoop.hdds.annotation.InterfaceAudience.Private;
+import org.apache.hadoop.hdds.annotation.InterfaceStability.Evolving;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Cache implementation for the table. Partial Table cache, where the DB state
+ * and cache state will not be same. Partial table cache holds entries until
+ * flush to DB happens.
+ */
+@Private
+@Evolving
+public class PartialTableCache<CACHEKEY extends CacheKey,
+ CACHEVALUE extends CacheValue> implements TableCache<CACHEKEY, CACHEVALUE> {
+
+ public static final Logger LOG =
+ LoggerFactory.getLogger(PartialTableCache.class);
+
+ private final Map<CACHEKEY, CACHEVALUE> cache;
+ private final NavigableSet<EpochEntry<CACHEKEY>> epochEntries;
+ private ExecutorService executorService;
+
+
+ public PartialTableCache() {
+ // We use concurrent Hash map for O(1) lookup for get API.
+ // During list operation for partial cache we anyway merge between DB and
+ // cache state. So entries in cache does not need to be in sorted order.
+
+ // And as concurrentHashMap computeIfPresent which is used by cleanup is
+ // atomic operation, and ozone level locks like bucket/volume locks
+ // protect updating same key, here it is not required to hold cache
+ // level locks during update/cleanup operation.
+
+ // 1. During update, it is caller responsibility to hold volume/bucket
+ // locks.
+ // 2. During cleanup which removes entry, while request is updating cache
+ // that should be guarded by concurrentHashMap guaranty.
+ cache = new ConcurrentHashMap<>();
+
+ epochEntries = new ConcurrentSkipListSet<>();
+ // Created a singleThreadExecutor, so one cleanup will be running at a
+ // time.
+ ThreadFactory build = new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat("PartialTableCache Cleanup Thread - %d").build();
+ executorService = Executors.newSingleThreadExecutor(build);
+ }
+
+ @Override
+ public CACHEVALUE get(CACHEKEY cachekey) {
+ return cache.get(cachekey);
+ }
+
+ @Override
+ public void loadInitial(CACHEKEY cacheKey, CACHEVALUE cacheValue) {
+ // Do nothing for full table cache.
+ }
+
+ @Override
+ public void put(CACHEKEY cacheKey, CACHEVALUE value) {
+ cache.put(cacheKey, value);
+ epochEntries.add(new EpochEntry<>(value.getEpoch(), cacheKey));
+ }
+
+ public void cleanup(List<Long> epochs) {
+ executorService.execute(() -> evictCache(epochs));
+ }
+
+ @Override
+ public int size() {
+ return cache.size();
+ }
+
+ @Override
+ public Iterator<Map.Entry<CACHEKEY, CACHEVALUE>> iterator() {
+ return cache.entrySet().iterator();
+ }
+
+ @VisibleForTesting
+ public void evictCache(List<Long> epochs) {
+ EpochEntry<CACHEKEY> currentEntry;
+ final AtomicBoolean removed = new AtomicBoolean();
+ CACHEKEY cachekey;
+ long lastEpoch = epochs.get(epochs.size() - 1);
+ for (Iterator<EpochEntry<CACHEKEY>> iterator = epochEntries.iterator();
+ iterator.hasNext();) {
+ currentEntry = iterator.next();
+ cachekey = currentEntry.getCachekey();
+ long currentEpoch = currentEntry.getEpoch();
+
+ // As ConcurrentHashMap computeIfPresent is atomic, there is no race
+ // condition between cache cleanup and requests updating same cache entry.
+
+ cache.computeIfPresent(cachekey, ((k, v) -> {
+ if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("CacheKey {} with epoch {} is removed from cache",
+ k.getCacheKey(), currentEpoch);
+ }
+ iterator.remove();
+ removed.set(true);
+ return null;
+ }
+ return v;
+ }));
+
+ // If currentEntry epoch is greater than last epoch provided, we have
+ // deleted all entries less than specified epoch. So, we can break.
+ if (currentEpoch > lastEpoch) {
+ break;
+ }
+
+ // When epoch entry is not removed, this might be a override entry in
+ // cache. Clean that epoch entry.
+ if (!removed.get()) {
+ if (LOG.isDebugEnabled()) {
Review comment:
LOG.isDebugEnabled check can be removed here.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -125,43 +136,44 @@ protected void evictCache(List<Long> epochs) {
currentEntry = iterator.next();
cachekey = currentEntry.getCachekey();
long currentEpoch = currentEntry.getEpoch();
- CacheValue cacheValue = cache.computeIfPresent(cachekey, ((k, v) -> {
- if (cleanupPolicy == CacheCleanupPolicy.MANUAL) {
- if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- iterator.remove();
- removed.set(true);
- return null;
- }
- } else if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- // Remove only entries which are marked for delete.
+
+ // Acquire lock to avoid race between cleanup and add to cache entry by
+ // client requests.
+ try {
+ lock.writeLock().lock();
+ cache.computeIfPresent(cachekey, ((k, v) -> {
if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())
&& v.getCacheValue() == null) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- removed.set(true);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("CacheKey {} with epoch {} is removed from cache",
+ k.getCacheKey(), currentEpoch);
+ }
iterator.remove();
+ removed.set(true);
return null;
}
+ return v;
+ }));
+ } finally {
+ lock.writeLock().unlock();
+ }
+
+ // If currentEntry epoch is greater than last epoch provided, we have
+ // deleted all entries less than specified epoch. So, we can break.
+ if (currentEpoch > lastEpoch) {
+ break;
+ }
+
+ // When epoch entry is not removed, this might be a override entry in
+ // cache. Clean that epoch entry.
+ if (!removed.get()) {
Review comment:
In the non-ratis OM cluster, we might get evictCache in non sorted order. Let's say we get evictCache (1,3,4). Then we might remove epoch entry 2 also even though it has not been flushed by DoubleBuffer, right?
Instead of having this removed boolean, would it be easier if we remove the epoch entry whenever it is there in evictCache list and only then? Something like this check before the cache.computeIfPresent()...
```
lock.writeLock().lock();
if (epochs.contains(currentEpoch) {
iterator.remove();
cache.computeIfPresent(cachekey, ((k, v) -> {
........
}
```
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -125,43 +136,44 @@ protected void evictCache(List<Long> epochs) {
currentEntry = iterator.next();
cachekey = currentEntry.getCachekey();
long currentEpoch = currentEntry.getEpoch();
- CacheValue cacheValue = cache.computeIfPresent(cachekey, ((k, v) -> {
- if (cleanupPolicy == CacheCleanupPolicy.MANUAL) {
- if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- iterator.remove();
- removed.set(true);
- return null;
- }
- } else if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- // Remove only entries which are marked for delete.
+
+ // Acquire lock to avoid race between cleanup and add to cache entry by
+ // client requests.
+ try {
+ lock.writeLock().lock();
+ cache.computeIfPresent(cachekey, ((k, v) -> {
if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())
&& v.getCacheValue() == null) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- removed.set(true);
+ if (LOG.isDebugEnabled()) {
Review comment:
I think with the new log4j, we do not need this check for isDebugEnabled(). Parameters will be evaluated only if Debug is enabled.
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -40,64 +41,74 @@
import org.slf4j.LoggerFactory;
/**
- * Cache implementation for the table. Depending on the cache clean up policy
- * this cache will be full cache or partial cache.
- *
- * If cache cleanup policy is set as {@link CacheCleanupPolicy#MANUAL},
- * this will be a partial cache.
- *
- * If cache cleanup policy is set as {@link CacheCleanupPolicy#NEVER},
- * this will be a full cache.
+ * Cache implementation for the table. Full Table cache, where the DB state
+ * and cache state will be same for these tables.
*/
@Private
@Evolving
-public class TableCacheImpl<CACHEKEY extends CacheKey,
+public class FullTableCache<CACHEKEY extends CacheKey,
CACHEVALUE extends CacheValue> implements TableCache<CACHEKEY, CACHEVALUE> {
public static final Logger LOG =
- LoggerFactory.getLogger(TableCacheImpl.class);
+ LoggerFactory.getLogger(FullTableCache.class);
private final Map<CACHEKEY, CACHEVALUE> cache;
private final NavigableSet<EpochEntry<CACHEKEY>> epochEntries;
private ExecutorService executorService;
- private CacheCleanupPolicy cleanupPolicy;
+ private final ReadWriteLock lock;
- public TableCacheImpl(CacheCleanupPolicy cleanupPolicy) {
-
+ public FullTableCache() {
// As for full table cache only we need elements to be inserted in sorted
- // manner, so that list will be easy. For other we can go with Hash map.
- if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- cache = new ConcurrentSkipListMap<>();
- } else {
- cache = new ConcurrentHashMap<>();
- }
+ // manner, so that list will be easy. But looks up have log(N) time
Review comment:
NIT: look ups
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#discussion_r547557324
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -125,43 +136,44 @@ protected void evictCache(List<Long> epochs) {
currentEntry = iterator.next();
cachekey = currentEntry.getCachekey();
long currentEpoch = currentEntry.getEpoch();
- CacheValue cacheValue = cache.computeIfPresent(cachekey, ((k, v) -> {
- if (cleanupPolicy == CacheCleanupPolicy.MANUAL) {
- if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- iterator.remove();
- removed.set(true);
- return null;
- }
- } else if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- // Remove only entries which are marked for delete.
+
+ // Acquire lock to avoid race between cleanup and add to cache entry by
+ // client requests.
+ try {
+ lock.writeLock().lock();
+ cache.computeIfPresent(cachekey, ((k, v) -> {
if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())
&& v.getCacheValue() == null) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- removed.set(true);
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("CacheKey {} with epoch {} is removed from cache",
+ k.getCacheKey(), currentEpoch);
+ }
iterator.remove();
+ removed.set(true);
return null;
}
+ return v;
+ }));
+ } finally {
+ lock.writeLock().unlock();
+ }
+
+ // If currentEntry epoch is greater than last epoch provided, we have
+ // deleted all entries less than specified epoch. So, we can break.
+ if (currentEpoch > lastEpoch) {
+ break;
+ }
+
+ // When epoch entry is not removed, this might be a override entry in
+ // cache. Clean that epoch entry.
+ if (!removed.get()) {
Review comment:
Good idea, updated code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] arp7 commented on pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
arp7 commented on pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#issuecomment-753406347
Good morning
On Thu, Dec 31, 2020, 3:17 PM Bharat Viswanadham <no...@github.com>
wrote:
> Thank You @hanishakoneru <https://github.com/hanishakoneru> for the
> review.
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/ozone/pull/1699#issuecomment-753223567>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAS5QYO5PJMSD3IDX3YGQTDSXUBAHANCNFSM4U3GUBFQ>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1699: HDDS-4583. TableCache Refactor to fix issues in cleanup never policy.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1699:
URL: https://github.com/apache/ozone/pull/1699#discussion_r547557407
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/cache/FullTableCache.java
##########
@@ -125,43 +136,44 @@ protected void evictCache(List<Long> epochs) {
currentEntry = iterator.next();
cachekey = currentEntry.getCachekey();
long currentEpoch = currentEntry.getEpoch();
- CacheValue cacheValue = cache.computeIfPresent(cachekey, ((k, v) -> {
- if (cleanupPolicy == CacheCleanupPolicy.MANUAL) {
- if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())) {
- LOG.debug("CacheKey {} with epoch {} is removed from cache",
- k.getCacheKey(), currentEpoch);
- iterator.remove();
- removed.set(true);
- return null;
- }
- } else if (cleanupPolicy == CacheCleanupPolicy.NEVER) {
- // Remove only entries which are marked for delete.
+
+ // Acquire lock to avoid race between cleanup and add to cache entry by
+ // client requests.
+ try {
+ lock.writeLock().lock();
+ cache.computeIfPresent(cachekey, ((k, v) -> {
if (v.getEpoch() == currentEpoch && epochs.contains(v.getEpoch())
&& v.getCacheValue() == null) {
Review comment:
Updated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org