You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/11/16 03:07:18 UTC
[GitHub] [hadoop] goiri commented on a diff in pull request #5138: HDFS-16844: Adds resilancy when StateStore gets exceptions.
goiri commented on code in PR #5138:
URL: https://github.com/apache/hadoop/pull/5138#discussion_r1023366622
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/CachedRecordStore.java:
##########
@@ -125,7 +125,6 @@ public boolean loadCache(boolean force) throws IOException {
} catch (IOException e) {
LOG.error("Cannot get \"{}\" records from the State Store",
getRecordClass().getSimpleName());
- this.initialized = false;
Review Comment:
Don't we want to keep this?
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestRouterState.java:
##########
@@ -82,4 +96,141 @@ public void testSerialization() throws IOException {
validateRecord(newRecord);
}
+
+ /**
+ * A mock StateStoreDriver that runs in memory and can cause errors.
+ */
+ public static class MockStateStoreDriver extends StateStoreBaseImpl {
+ boolean giveErrors = false;
+ boolean initialized = false;
+ Map<String, Map<String, BaseRecord>> valueMap = new HashMap<>();
+
+ @Override
+ public boolean initDriver() {
+ initialized = true;
+ return true;
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean initRecordStorage(String className,
+ Class<T> clazz) {
+ return true;
+ }
+
+ @Override
+ public boolean isDriverReady() {
+ return initialized;
+ }
+
+ @Override
+ public void close() throws Exception {
+ valueMap.clear();
+ initialized = false;
+ }
+
+ private void checkErrors() throws IOException {
+ if (giveErrors) {
+ throw new IOException("Induced errors");
+ }
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ public <T extends BaseRecord> QueryResult get(Class<T> clazz) throws IOException {
+ checkErrors();
+ Map<String, BaseRecord> map = valueMap.get(StateStoreUtils.getRecordName(clazz));
+ List<BaseRecord> results = map != null
+ ? new ArrayList<>(map.values()) : new ArrayList<>();
+ return new QueryResult<>(results, System.currentTimeMillis());
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean putAll(List<T> records,
+ boolean allowUpdate,
+ boolean errorIfExists)
+ throws IOException {
+ checkErrors();
+ for (T record: records) {
+ Map<String, BaseRecord> map =
+ valueMap.computeIfAbsent(StateStoreUtils.getRecordName(record.getClass()),
+ k -> new HashMap<>());
+ String key = record.getPrimaryKey();
+ BaseRecord oldRecord = map.get(key);
+ if (oldRecord == null || allowUpdate) {
+ map.put(key, record);
+ } else if (errorIfExists) {
+ throw new IOException("Record already exists for " + record.getClass()
+ + ": " + key);
+ }
+ }
+ return true;
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean removeAll(Class<T> clazz) throws IOException {
+ checkErrors();
+ valueMap.remove(StateStoreUtils.getRecordName(clazz));
Review Comment:
Should we do return != null?
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestRouterState.java:
##########
@@ -82,4 +96,141 @@ public void testSerialization() throws IOException {
validateRecord(newRecord);
}
+
+ /**
+ * A mock StateStoreDriver that runs in memory and can cause errors.
+ */
+ public static class MockStateStoreDriver extends StateStoreBaseImpl {
Review Comment:
Maybe is cleaner to put it into a separate file.
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestRouterState.java:
##########
@@ -82,4 +96,141 @@ public void testSerialization() throws IOException {
validateRecord(newRecord);
}
+
+ /**
+ * A mock StateStoreDriver that runs in memory and can cause errors.
+ */
+ public static class MockStateStoreDriver extends StateStoreBaseImpl {
+ boolean giveErrors = false;
+ boolean initialized = false;
+ Map<String, Map<String, BaseRecord>> valueMap = new HashMap<>();
+
+ @Override
+ public boolean initDriver() {
+ initialized = true;
+ return true;
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean initRecordStorage(String className,
+ Class<T> clazz) {
+ return true;
+ }
+
+ @Override
+ public boolean isDriverReady() {
+ return initialized;
+ }
+
+ @Override
+ public void close() throws Exception {
+ valueMap.clear();
+ initialized = false;
+ }
+
+ private void checkErrors() throws IOException {
+ if (giveErrors) {
+ throw new IOException("Induced errors");
+ }
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ public <T extends BaseRecord> QueryResult get(Class<T> clazz) throws IOException {
+ checkErrors();
+ Map<String, BaseRecord> map = valueMap.get(StateStoreUtils.getRecordName(clazz));
+ List<BaseRecord> results = map != null
Review Comment:
With the new line 100 limit you could fit this into a single one.
Same thing in a couple more places.
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/store/records/TestRouterState.java:
##########
@@ -82,4 +96,141 @@ public void testSerialization() throws IOException {
validateRecord(newRecord);
}
+
+ /**
+ * A mock StateStoreDriver that runs in memory and can cause errors.
+ */
+ public static class MockStateStoreDriver extends StateStoreBaseImpl {
+ boolean giveErrors = false;
+ boolean initialized = false;
+ Map<String, Map<String, BaseRecord>> valueMap = new HashMap<>();
+
+ @Override
+ public boolean initDriver() {
+ initialized = true;
+ return true;
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean initRecordStorage(String className,
+ Class<T> clazz) {
+ return true;
+ }
+
+ @Override
+ public boolean isDriverReady() {
+ return initialized;
+ }
+
+ @Override
+ public void close() throws Exception {
+ valueMap.clear();
+ initialized = false;
+ }
+
+ private void checkErrors() throws IOException {
+ if (giveErrors) {
+ throw new IOException("Induced errors");
+ }
+ }
+
+ @Override
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ public <T extends BaseRecord> QueryResult get(Class<T> clazz) throws IOException {
+ checkErrors();
+ Map<String, BaseRecord> map = valueMap.get(StateStoreUtils.getRecordName(clazz));
+ List<BaseRecord> results = map != null
+ ? new ArrayList<>(map.values()) : new ArrayList<>();
+ return new QueryResult<>(results, System.currentTimeMillis());
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean putAll(List<T> records,
+ boolean allowUpdate,
+ boolean errorIfExists)
+ throws IOException {
+ checkErrors();
+ for (T record: records) {
+ Map<String, BaseRecord> map =
+ valueMap.computeIfAbsent(StateStoreUtils.getRecordName(record.getClass()),
+ k -> new HashMap<>());
+ String key = record.getPrimaryKey();
+ BaseRecord oldRecord = map.get(key);
+ if (oldRecord == null || allowUpdate) {
+ map.put(key, record);
+ } else if (errorIfExists) {
+ throw new IOException("Record already exists for " + record.getClass()
+ + ": " + key);
+ }
+ }
+ return true;
+ }
+
+ @Override
+ public <T extends BaseRecord> boolean removeAll(Class<T> clazz) throws IOException {
+ checkErrors();
+ valueMap.remove(StateStoreUtils.getRecordName(clazz));
+ return true;
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public <T extends BaseRecord> int remove(Class<T> clazz,
+ Query<T> query)
+ throws IOException {
+ checkErrors();
+ int result = 0;
+ Map<String, BaseRecord> map =
+ valueMap.get(StateStoreUtils.getRecordName(clazz));
+ if (map != null) {
+ for (Iterator<BaseRecord> itr = map.values().iterator(); itr.hasNext(); ) {
+ BaseRecord record = itr.next();
+ if (query.matches((T) record)) {
+ itr.remove();
+ result += 1;
+ }
+ }
+ }
+ return result;
+ }
+ }
+
+ @Test
+ public void testStateStoreResilience() throws Exception {
+ StateStoreService service = new StateStoreService();
+ Configuration conf = new Configuration();
+ conf.setClass(RBFConfigKeys.FEDERATION_STORE_DRIVER_CLASS,
+ MockStateStoreDriver.class,
Review Comment:
Wouldn't it make sense to just use StateStoreFileImpl?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org