From a890c9ceb0c025ac80c97ae97beceeddaa2f33f0 Mon Sep 17 00:00:00 2001 From: tejaskriya Date: Tue, 17 Dec 2024 12:13:08 +0530 Subject: [PATCH 1/5] HDDS-11746. Add option to pass DBDefinition for RDPParser commands --- .../DatanodeSchemaOneDBDefinition.java | 5 +++ .../DatanodeSchemaThreeDBDefinition.java | 5 +++ .../DatanodeSchemaTwoDBDefinition.java | 5 +++ .../hdds/scm/metadata/SCMDBDefinition.java | 4 +++ .../ozone/debug/DBDefinitionFactory.java | 31 +++++++++++++++++++ .../hadoop/ozone/debug/ldb/DBScanner.java | 2 +- .../hadoop/ozone/debug/ldb/RDBParser.java | 8 +++++ .../hadoop/ozone/debug/ldb/ValueSchema.java | 6 ++-- 8 files changed, 62 insertions(+), 4 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java index bd1c0fb368a..992a8635f64 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java @@ -19,6 +19,7 @@ import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.LongCodec; import org.apache.hadoop.hdds.utils.CollectionUtils; @@ -79,6 +80,10 @@ public DatanodeSchemaOneDBDefinition(String dbPath, super(dbPath, config); } + public DatanodeSchemaOneDBDefinition(String dbPath) { + super(dbPath, new OzoneConfiguration()); + } + @Override public DBColumnFamilyDefinition getBlockDataColumnFamily() { diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java index 10537ca6f2d..66faa02a422 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java @@ -19,6 +19,7 @@ import com.google.common.primitives.Longs; import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; @@ -124,6 +125,10 @@ public DatanodeSchemaThreeDBDefinition(String dbPath, LAST_CHUNK_INFO.setCfOptions(cfOptions); } + public DatanodeSchemaThreeDBDefinition(String dbPath) { + super(dbPath, new OzoneConfiguration()); + } + @Override public Map> getMap() { return COLUMN_FAMILIES; diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java index bf6b1d0a29c..59bc6cd4d82 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java @@ -18,6 +18,7 @@ package org.apache.hadoop.ozone.container.metadata; import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; import org.apache.hadoop.hdds.utils.db.FixedLengthStringCodec; @@ -79,6 +80,10 @@ public DatanodeSchemaTwoDBDefinition(String dbPath, super(dbPath, config); } + public DatanodeSchemaTwoDBDefinition(String dbPath) { + super(dbPath, new OzoneConfiguration()); + } + private static final Map> COLUMN_FAMILIES = DBColumnFamilyDefinition.newUnmodifiableMap( BLOCK_DATA, diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java index ea86fa154af..c372faf4781 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java @@ -138,6 +138,10 @@ protected SCMDBDefinition(Map> map) { super(map); } + public SCMDBDefinition() { + this(COLUMN_FAMILIES); + } + @Override public String getName() { return "scm.db"; diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java index ca79aa41fa4..7eb70b3279f 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java @@ -18,6 +18,7 @@ package org.apache.hadoop.ozone.debug; +import java.lang.reflect.Constructor; import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; @@ -98,6 +99,36 @@ public static DBDefinition getDefinition(Path dbPath, return getDefinition(dbName); } + public static DBDefinition getDefinition(Path dbName, ConfigurationSource config, String overrideDBDef) { + if (overrideDBDef == null) { + return getDefinition(dbName, config); + } + try { + Class clazz = Class.forName(overrideDBDef); + if (DBDefinition.class.isAssignableFrom(clazz)) { + DBDefinition instance; + try { + Constructor noParamConstructor = clazz.getDeclaredConstructor(); + noParamConstructor.setAccessible(true); + instance = (DBDefinition) noParamConstructor.newInstance(); + } catch (NoSuchMethodException e) { + Constructor stringParamConstructor = clazz.getDeclaredConstructor(String.class); + stringParamConstructor.setAccessible(true); + instance = (DBDefinition) stringParamConstructor.newInstance(dbName.toAbsolutePath().toString()); + } + return instance; + } else { + System.err.println("Class does not implement DBDefinition: " + overrideDBDef); + } + } catch (ClassNotFoundException e) { + System.err.println("Class not found: " + overrideDBDef); + } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | + SecurityException | java.lang.reflect.InvocationTargetException e) { + System.err.println("Error creating instance: " + e.getMessage()); + } + return null; + } + private static DBDefinition getReconDBDefinition(String dbName) { if (dbName.startsWith(RECON_CONTAINER_KEY_DB)) { return new ReconDBDefinition(dbName); diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/DBScanner.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/DBScanner.java index 6fbbd1a3083..96d8b68347f 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/DBScanner.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/DBScanner.java @@ -581,7 +581,7 @@ private boolean printTable(List columnFamilyHandleList, dbPath = removeTrailingSlashIfNeeded(dbPath); DBDefinitionFactory.setDnDBSchemaVersion(dnDBSchemaVersion); DBDefinition dbDefinition = DBDefinitionFactory.getDefinition( - Paths.get(dbPath), new OzoneConfiguration()); + Paths.get(dbPath), new OzoneConfiguration(), parent.getDbDefinition()); if (dbDefinition == null) { err().println("Error: Incorrect DB Path"); return false; diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/RDBParser.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/RDBParser.java index 4e945c7c418..b51ed9ec84f 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/RDBParser.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/RDBParser.java @@ -51,6 +51,10 @@ public class RDBParser implements Callable, DebugSubcommand { description = "Database File Path") private String dbPath; + @CommandLine.Option(names = {"--schema"}, + description = "DBDefinition of the database") + private String dbDefinition; + public String getDbPath() { return dbPath; } @@ -59,6 +63,10 @@ public void setDbPath(String dbPath) { this.dbPath = dbPath; } + public String getDbDefinition() { + return dbDefinition; + } + @Override public Void call() throws Exception { GenericCli.missingSubcommand(spec); diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/ValueSchema.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/ValueSchema.java index 4b8eb3b3208..d5f6ffd33fe 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/ValueSchema.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ldb/ValueSchema.java @@ -86,7 +86,7 @@ public Void call() throws Exception { String dbPath = parent.getDbPath(); Map fields = new HashMap<>(); - success = getValueFields(dbPath, fields, depth, tableName, dnDBSchemaVersion); + success = getValueFields(dbPath, fields, depth, tableName, dnDBSchemaVersion, parent.getDbDefinition()); out().println(JsonUtils.toJsonStringWithDefaultPrettyPrinter(fields)); @@ -100,11 +100,11 @@ public Void call() throws Exception { } public static boolean getValueFields(String dbPath, Map valueSchema, int d, String table, - String dnDBSchemaVersion) { + String dnDBSchemaVersion, String dbDef) { dbPath = removeTrailingSlashIfNeeded(dbPath); DBDefinitionFactory.setDnDBSchemaVersion(dnDBSchemaVersion); - DBDefinition dbDefinition = DBDefinitionFactory.getDefinition(Paths.get(dbPath), new OzoneConfiguration()); + DBDefinition dbDefinition = DBDefinitionFactory.getDefinition(Paths.get(dbPath), new OzoneConfiguration(), dbDef); if (dbDefinition == null) { err().println("Error: Incorrect DB Path"); return false; From 8e43cec9a0b8b025584e38c36b196543e57ffcc7 Mon Sep 17 00:00:00 2001 From: tejaskriya Date: Mon, 6 Jan 2025 20:36:07 +0530 Subject: [PATCH 2/5] Add tests to check arbitrary schema option --- .../apache/hadoop/ozone/debug/TestLDBCli.java | 69 +++++++++++++++++++ .../ozone/debug/DBDefinitionFactory.java | 6 +- .../ozone/debug/TestDBDefinitionFactory.java | 50 ++++++++++++++ 3 files changed, 122 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java index aac55367adc..1871c46b1a6 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java @@ -26,9 +26,12 @@ import org.apache.hadoop.hdds.client.ReplicationConfig; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.utils.db.DBDefinition; +import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.DBStore; import org.apache.hadoop.hdds.utils.db.DBStoreBuilder; import org.apache.hadoop.hdds.utils.db.FixedLengthStringCodec; +import org.apache.hadoop.hdds.utils.db.StringCodec; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.ozone.ClientVersion; import org.apache.hadoop.ozone.OzoneConsts; @@ -80,6 +83,7 @@ public class TestLDBCli { private static final String KEY_TABLE = "keyTable"; private static final String BLOCK_DATA = "block_data"; public static final String PIPELINES = "pipelines"; + public static final String DUMMY_DB = "anotherKeyTable"; private static final ObjectMapper MAPPER = new ObjectMapper(); private OzoneConfiguration conf; private DBStore dbStore; @@ -410,6 +414,40 @@ void testSchemaCommand() throws IOException { assertEquals("", stderr.toString()); } + @Test + void testCommandsWithDBDefOverride() throws IOException { + // Prepare dummy table + prepareTable(DUMMY_DB, true); + + // Prepare args for value-schema command + List completeScanArgs = new ArrayList<>(Arrays.asList( + "--db", dbStore.getDbLocation().getAbsolutePath(), + "--schema", DummyDBDefinition.class.getName(), + "value-schema", + "--column-family", DummyDBDefinition.ANOTHER_KEY_TABLE_NAME)); + + int exitCode = cmd.execute(completeScanArgs.toArray(new String[0])); + assertEquals(0, exitCode, stderr.toString()); + Pattern p = Pattern.compile(".*String.*String.*", Pattern.MULTILINE); + Matcher m = p.matcher(stdout.toString()); + assertTrue(m.find()); + assertEquals("", stderr.toString()); + + // Prepare args for scan command + completeScanArgs = new ArrayList<>(Arrays.asList( + "--db", dbStore.getDbLocation().getAbsolutePath(), + "--schema", DummyDBDefinition.class.getName(), + "scan", + "--column-family", DummyDBDefinition.ANOTHER_KEY_TABLE_NAME)); + + exitCode = cmd.execute(completeScanArgs.toArray(new String[0])); + assertEquals(0, exitCode, stderr.toString()); + p = Pattern.compile(".*random-key.*random-value.*", Pattern.MULTILINE); + m = p.matcher(stdout.toString()); + assertTrue(m.find()); + assertEquals("", stderr.toString()); + } + /** * Converts String input to a Map and compares to the given Map input. * @param expected expected result Map @@ -473,11 +511,23 @@ private void prepareTable(String tableName, boolean schemaV3) } } break; + case PIPELINES: // Empty table dbStore = DBStoreBuilder.newBuilder(conf).setName("scm.db") .setPath(tempDir.toPath()).addTable(PIPELINES).build(); break; + + case DUMMY_DB: + dbStore = DBStoreBuilder.newBuilder(new OzoneConfiguration()) + .setName("another.db") + .setPath(tempDir.toPath()) + .addTable(DummyDBDefinition.ANOTHER_KEY_TABLE_NAME) + .build(); + dbStore.getTable(DummyDBDefinition.ANOTHER_KEY_TABLE_NAME, String.class, String.class) + .put("random-key", "random-value"); + break; + default: throw new IllegalArgumentException("Unsupported table: " + tableName); } @@ -512,4 +562,23 @@ private static Map toMap(Object obj) throws IOException { return MAPPER.readValue(json, new TypeReference>() { }); } + public static class DummyDBDefinition extends DBDefinition.WithMap { + public static String ANOTHER_KEY_TABLE_NAME = "anotherKeyTable"; + public static final DBColumnFamilyDefinition ANOTHER_KEY_TABLE + = new DBColumnFamilyDefinition<>(ANOTHER_KEY_TABLE_NAME, StringCodec.get(), StringCodec.get()); + private static final Map> + COLUMN_FAMILIES = DBColumnFamilyDefinition.newUnmodifiableMap(ANOTHER_KEY_TABLE); + protected DummyDBDefinition() { + super(COLUMN_FAMILIES); + } + @Override + public String getName() { + return "another.db"; + } + @Override + public String getLocationConfigKey() { + return ""; + } + } + } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java index 7eb70b3279f..adcedd78800 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java @@ -99,9 +99,9 @@ public static DBDefinition getDefinition(Path dbPath, return getDefinition(dbName); } - public static DBDefinition getDefinition(Path dbName, ConfigurationSource config, String overrideDBDef) { + public static DBDefinition getDefinition(Path dbPath, ConfigurationSource config, String overrideDBDef) { if (overrideDBDef == null) { - return getDefinition(dbName, config); + return getDefinition(dbPath, config); } try { Class clazz = Class.forName(overrideDBDef); @@ -114,7 +114,7 @@ public static DBDefinition getDefinition(Path dbName, ConfigurationSource config } catch (NoSuchMethodException e) { Constructor stringParamConstructor = clazz.getDeclaredConstructor(String.class); stringParamConstructor.setAccessible(true); - instance = (DBDefinition) stringParamConstructor.newInstance(dbName.toAbsolutePath().toString()); + instance = (DBDefinition) stringParamConstructor.newInstance(dbPath.toAbsolutePath().toString()); } return instance; } else { diff --git a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java index 5f0be7859d4..617b222e2a0 100644 --- a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java @@ -18,9 +18,13 @@ package org.apache.hadoop.ozone.debug; +import java.lang.reflect.Constructor; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.HashSet; +import java.util.Set; + import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.metadata.SCMDBDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; @@ -34,8 +38,12 @@ import static org.apache.hadoop.ozone.recon.ReconConstants.RECON_CONTAINER_KEY_DB; import static org.apache.hadoop.ozone.recon.ReconConstants.RECON_OM_SNAPSHOT_DB; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; +import org.reflections.Reflections; /** * Simple factory unit test. @@ -75,4 +83,46 @@ public void testGetDefinition() { definition = DBDefinitionFactory.getDefinition(dbPath, conf); assertInstanceOf(DatanodeSchemaThreeDBDefinition.class, definition); } + + @Test + public void testGetDefinitionWithOverride() { + final OzoneConfiguration conf = new OzoneConfiguration(); + Path dbPath = Paths.get("another.db"); + DBDefinition definition = DBDefinitionFactory.getDefinition(dbPath, conf, OMDBDefinition.class.getName()); + assertInstanceOf(OMDBDefinition.class, definition); + } + + /* + * Test to ensure that any DBDefinition has a default constructor or a constructor with 1 parameter. + * This is needed for ldb tools to run with arbitrary DB definitions. + */ + @Test + public void testAllDBDefinitionsHaveCorrectConstructor() { + Set> withMapSubclasses = new HashSet<>(); + try { + Reflections reflections = new Reflections("org.apache.hadoop"); + withMapSubclasses = reflections.getSubTypesOf(DBDefinition.WithMap.class); + } catch (Exception e) { + fail("Error while finding subclasses: " + e.getMessage()); + } + assertFalse(withMapSubclasses.isEmpty(), "No classes found extending DBDefinition.WithMap"); + System.out.println(withMapSubclasses); + + // Check constructors for each subclass + for (Class clazz : withMapSubclasses) { + Constructor[] constructors = clazz.getDeclaredConstructors(); + boolean hasValidConstructor = false; + + for (Constructor constructor : constructors) { + int paramCount = constructor.getParameterCount(); + if (paramCount == 0 || paramCount == 1) { + hasValidConstructor = true; + break; + } + } + + assertTrue(hasValidConstructor, + "Class " + clazz.getName() + " does not have a valid constructor (default or single parameter)"); + } + } } From 55f8ab390a69acf4cc0bcca090f58061c6a2abbd Mon Sep 17 00:00:00 2001 From: tejaskriya Date: Mon, 6 Jan 2025 20:54:25 +0530 Subject: [PATCH 3/5] Checkstyle and findBugs fix --- .../org/apache/hadoop/ozone/debug/TestLDBCli.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java index 1871c46b1a6..df95f0dd71f 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java @@ -84,6 +84,7 @@ public class TestLDBCli { private static final String BLOCK_DATA = "block_data"; public static final String PIPELINES = "pipelines"; public static final String DUMMY_DB = "anotherKeyTable"; + public static final String ANOTHER_KEY_TABLE_NAME = "anotherKeyTable"; private static final ObjectMapper MAPPER = new ObjectMapper(); private OzoneConfiguration conf; private DBStore dbStore; @@ -424,7 +425,7 @@ void testCommandsWithDBDefOverride() throws IOException { "--db", dbStore.getDbLocation().getAbsolutePath(), "--schema", DummyDBDefinition.class.getName(), "value-schema", - "--column-family", DummyDBDefinition.ANOTHER_KEY_TABLE_NAME)); + "--column-family", ANOTHER_KEY_TABLE_NAME)); int exitCode = cmd.execute(completeScanArgs.toArray(new String[0])); assertEquals(0, exitCode, stderr.toString()); @@ -438,7 +439,7 @@ void testCommandsWithDBDefOverride() throws IOException { "--db", dbStore.getDbLocation().getAbsolutePath(), "--schema", DummyDBDefinition.class.getName(), "scan", - "--column-family", DummyDBDefinition.ANOTHER_KEY_TABLE_NAME)); + "--column-family", ANOTHER_KEY_TABLE_NAME)); exitCode = cmd.execute(completeScanArgs.toArray(new String[0])); assertEquals(0, exitCode, stderr.toString()); @@ -522,9 +523,9 @@ private void prepareTable(String tableName, boolean schemaV3) dbStore = DBStoreBuilder.newBuilder(new OzoneConfiguration()) .setName("another.db") .setPath(tempDir.toPath()) - .addTable(DummyDBDefinition.ANOTHER_KEY_TABLE_NAME) + .addTable(ANOTHER_KEY_TABLE_NAME) .build(); - dbStore.getTable(DummyDBDefinition.ANOTHER_KEY_TABLE_NAME, String.class, String.class) + dbStore.getTable(ANOTHER_KEY_TABLE_NAME, String.class, String.class) .put("random-key", "random-value"); break; @@ -562,8 +563,10 @@ private static Map toMap(Object obj) throws IOException { return MAPPER.readValue(json, new TypeReference>() { }); } + /** + * New DBDefinition to test arbitrary schemas for ldb commands. + */ public static class DummyDBDefinition extends DBDefinition.WithMap { - public static String ANOTHER_KEY_TABLE_NAME = "anotherKeyTable"; public static final DBColumnFamilyDefinition ANOTHER_KEY_TABLE = new DBColumnFamilyDefinition<>(ANOTHER_KEY_TABLE_NAME, StringCodec.get(), StringCodec.get()); private static final Map> From 91d1dab0917d23785b7caef03cd4b9f613db0d7e Mon Sep 17 00:00:00 2001 From: tejaskriya Date: Wed, 8 Jan 2025 12:13:07 +0530 Subject: [PATCH 4/5] remove added constructors and check for 4 types of methods, any one of which should exist --- .../DatanodeSchemaOneDBDefinition.java | 5 -- .../DatanodeSchemaThreeDBDefinition.java | 5 -- .../DatanodeSchemaTwoDBDefinition.java | 5 -- .../hdds/scm/metadata/SCMDBDefinition.java | 4 -- .../ozone/debug/DBDefinitionFactory.java | 51 ++++++++++++++----- .../ozone/debug/TestDBDefinitionFactory.java | 41 ++++++++------- 6 files changed, 59 insertions(+), 52 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java index 992a8635f64..bd1c0fb368a 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaOneDBDefinition.java @@ -19,7 +19,6 @@ import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.conf.ConfigurationSource; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.LongCodec; import org.apache.hadoop.hdds.utils.CollectionUtils; @@ -80,10 +79,6 @@ public DatanodeSchemaOneDBDefinition(String dbPath, super(dbPath, config); } - public DatanodeSchemaOneDBDefinition(String dbPath) { - super(dbPath, new OzoneConfiguration()); - } - @Override public DBColumnFamilyDefinition getBlockDataColumnFamily() { diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java index 66faa02a422..10537ca6f2d 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java @@ -19,7 +19,6 @@ import com.google.common.primitives.Longs; import org.apache.hadoop.hdds.conf.ConfigurationSource; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; @@ -125,10 +124,6 @@ public DatanodeSchemaThreeDBDefinition(String dbPath, LAST_CHUNK_INFO.setCfOptions(cfOptions); } - public DatanodeSchemaThreeDBDefinition(String dbPath) { - super(dbPath, new OzoneConfiguration()); - } - @Override public Map> getMap() { return COLUMN_FAMILIES; diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java index 59bc6cd4d82..bf6b1d0a29c 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaTwoDBDefinition.java @@ -18,7 +18,6 @@ package org.apache.hadoop.ozone.container.metadata; import org.apache.hadoop.hdds.conf.ConfigurationSource; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.utils.db.DBColumnFamilyDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; import org.apache.hadoop.hdds.utils.db.FixedLengthStringCodec; @@ -80,10 +79,6 @@ public DatanodeSchemaTwoDBDefinition(String dbPath, super(dbPath, config); } - public DatanodeSchemaTwoDBDefinition(String dbPath) { - super(dbPath, new OzoneConfiguration()); - } - private static final Map> COLUMN_FAMILIES = DBColumnFamilyDefinition.newUnmodifiableMap( BLOCK_DATA, diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java index c372faf4781..ea86fa154af 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/metadata/SCMDBDefinition.java @@ -138,10 +138,6 @@ protected SCMDBDefinition(Map> map) { super(map); } - public SCMDBDefinition() { - this(COLUMN_FAMILIES); - } - @Override public String getName() { return "scm.db"; diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java index adcedd78800..9b31ab64088 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java @@ -19,10 +19,12 @@ package org.apache.hadoop.ozone.debug; import java.lang.reflect.Constructor; +import java.lang.reflect.Method; import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicReference; @@ -40,6 +42,7 @@ import com.amazonaws.services.kms.model.InvalidArnException; import com.google.common.base.Preconditions; +import org.apache.ratis.util.function.CheckedSupplier; import static org.apache.hadoop.ozone.OzoneConsts.OM_DB_NAME; import static org.apache.hadoop.ozone.recon.ReconConstants.RECON_CONTAINER_KEY_DB; @@ -99,6 +102,32 @@ public static DBDefinition getDefinition(Path dbPath, return getDefinition(dbName); } + public static List> getFactories( + Class clazz, String dbPathString, ConfigurationSource config) { + return Arrays.asList( + () -> { + Method factoryMethod = clazz.getDeclaredMethod("get"); + factoryMethod.setAccessible(true); + return (DBDefinition) factoryMethod.invoke(clazz); + }, + () -> { + Constructor constructor = clazz.getDeclaredConstructor(); + constructor.setAccessible(true); + return (DBDefinition) constructor.newInstance(); + }, + () -> { + Constructor constructor = clazz.getDeclaredConstructor(String.class); + constructor.setAccessible(true); + return (DBDefinition) constructor.newInstance(dbPathString); + }, + () -> { + Constructor constructor = clazz.getDeclaredConstructor(String.class, ConfigurationSource.class); + constructor.setAccessible(true); + return (DBDefinition) constructor.newInstance(dbPathString, config); + } + ); + } + public static DBDefinition getDefinition(Path dbPath, ConfigurationSource config, String overrideDBDef) { if (overrideDBDef == null) { return getDefinition(dbPath, config); @@ -106,27 +135,21 @@ public static DBDefinition getDefinition(Path dbPath, ConfigurationSource config try { Class clazz = Class.forName(overrideDBDef); if (DBDefinition.class.isAssignableFrom(clazz)) { - DBDefinition instance; - try { - Constructor noParamConstructor = clazz.getDeclaredConstructor(); - noParamConstructor.setAccessible(true); - instance = (DBDefinition) noParamConstructor.newInstance(); - } catch (NoSuchMethodException e) { - Constructor stringParamConstructor = clazz.getDeclaredConstructor(String.class); - stringParamConstructor.setAccessible(true); - instance = (DBDefinition) stringParamConstructor.newInstance(dbPath.toAbsolutePath().toString()); + String dbPathString = dbPath.toAbsolutePath().toString(); + for (CheckedSupplier factory : getFactories(clazz, dbPathString, config)) { + try { + return factory.get(); + } catch (Exception ignored) { + } } - return instance; + throw new IllegalArgumentException("Could not get instance of " + overrideDBDef); } else { System.err.println("Class does not implement DBDefinition: " + overrideDBDef); } } catch (ClassNotFoundException e) { System.err.println("Class not found: " + overrideDBDef); - } catch (InstantiationException | IllegalAccessException | NoSuchMethodException | - SecurityException | java.lang.reflect.InvocationTargetException e) { - System.err.println("Error creating instance: " + e.getMessage()); } - return null; + throw new IllegalArgumentException("Incorrect DBDefinition class."); } private static DBDefinition getReconDBDefinition(String dbName) { diff --git a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java index 617b222e2a0..26a4932433c 100644 --- a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java @@ -18,11 +18,11 @@ package org.apache.hadoop.ozone.debug; -import java.lang.reflect.Constructor; import java.nio.file.Path; import java.nio.file.Paths; import java.util.HashSet; +import java.util.List; import java.util.Set; import org.apache.hadoop.hdds.conf.OzoneConfiguration; @@ -42,6 +42,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.apache.ratis.util.function.CheckedSupplier; import org.junit.jupiter.api.Test; import org.reflections.Reflections; @@ -98,31 +99,33 @@ public void testGetDefinitionWithOverride() { */ @Test public void testAllDBDefinitionsHaveCorrectConstructor() { - Set> withMapSubclasses = new HashSet<>(); + Set> subclasses = new HashSet<>(); try { Reflections reflections = new Reflections("org.apache.hadoop"); - withMapSubclasses = reflections.getSubTypesOf(DBDefinition.WithMap.class); + subclasses.addAll(reflections.getSubTypesOf(DBDefinition.class)); + subclasses.remove(Class.forName("org.apache.hadoop.hdds.utils.db.DBDefinition$WithMap")); + subclasses.remove(Class.forName("org.apache.hadoop.hdds.utils.db.DBDefinition$WithMapInterface")); + subclasses.remove(Class.forName( + "org.apache.hadoop.ozone.container.metadata.AbstractDatanodeDBDefinition")); } catch (Exception e) { fail("Error while finding subclasses: " + e.getMessage()); } - assertFalse(withMapSubclasses.isEmpty(), "No classes found extending DBDefinition.WithMap"); - System.out.println(withMapSubclasses); - - // Check constructors for each subclass - for (Class clazz : withMapSubclasses) { - Constructor[] constructors = clazz.getDeclaredConstructors(); - boolean hasValidConstructor = false; - - for (Constructor constructor : constructors) { - int paramCount = constructor.getParameterCount(); - if (paramCount == 0 || paramCount == 1) { - hasValidConstructor = true; - break; + assertFalse(subclasses.isEmpty(), "No classes found extending DBDefinition"); + + for (Class clazz : subclasses) { + List> factories = + DBDefinitionFactory.getFactories(clazz, "testDbPath", new OzoneConfiguration()); + boolean hasValidFactory = factories.stream().anyMatch(factory -> { + try { + factory.get(); + return true; + } catch (Exception e) { + return false; } - } + }); - assertTrue(hasValidConstructor, - "Class " + clazz.getName() + " does not have a valid constructor (default or single parameter)"); + assertTrue(hasValidFactory, + "Class " + clazz.getName() + " does not have a valid constructor or factory method."); } } } From b4c89496b6cc25e39aa92485b17089cbc79f32b4 Mon Sep 17 00:00:00 2001 From: tejaskriya Date: Thu, 9 Jan 2025 17:09:53 +0530 Subject: [PATCH 5/5] Rearrange constructor list and improve test --- .../ozone/debug/DBDefinitionFactory.java | 20 +++++++++---------- .../ozone/debug/TestDBDefinitionFactory.java | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java index 9b31ab64088..b53021fb77a 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBDefinitionFactory.java @@ -102,18 +102,13 @@ public static DBDefinition getDefinition(Path dbPath, return getDefinition(dbName); } - public static List> getFactories( + static List> getFactories( Class clazz, String dbPathString, ConfigurationSource config) { return Arrays.asList( () -> { - Method factoryMethod = clazz.getDeclaredMethod("get"); - factoryMethod.setAccessible(true); - return (DBDefinition) factoryMethod.invoke(clazz); - }, - () -> { - Constructor constructor = clazz.getDeclaredConstructor(); + Constructor constructor = clazz.getDeclaredConstructor(String.class, ConfigurationSource.class); constructor.setAccessible(true); - return (DBDefinition) constructor.newInstance(); + return (DBDefinition) constructor.newInstance(dbPathString, config); }, () -> { Constructor constructor = clazz.getDeclaredConstructor(String.class); @@ -121,9 +116,14 @@ public static List> getFactories( return (DBDefinition) constructor.newInstance(dbPathString); }, () -> { - Constructor constructor = clazz.getDeclaredConstructor(String.class, ConfigurationSource.class); + Method factoryMethod = clazz.getDeclaredMethod("get"); + factoryMethod.setAccessible(true); + return (DBDefinition) factoryMethod.invoke(clazz); + }, + () -> { + Constructor constructor = clazz.getDeclaredConstructor(); constructor.setAccessible(true); - return (DBDefinition) constructor.newInstance(dbPathString, config); + return (DBDefinition) constructor.newInstance(); } ); } diff --git a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java index 26a4932433c..dbde2c79feb 100644 --- a/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java +++ b/hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/debug/TestDBDefinitionFactory.java @@ -28,6 +28,7 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.scm.metadata.SCMDBDefinition; import org.apache.hadoop.hdds.utils.db.DBDefinition; +import org.apache.hadoop.ozone.container.metadata.AbstractDatanodeDBDefinition; import org.apache.hadoop.ozone.container.metadata.DatanodeSchemaOneDBDefinition; import org.apache.hadoop.ozone.container.metadata.DatanodeSchemaThreeDBDefinition; import org.apache.hadoop.ozone.container.metadata.DatanodeSchemaTwoDBDefinition; @@ -101,12 +102,11 @@ public void testGetDefinitionWithOverride() { public void testAllDBDefinitionsHaveCorrectConstructor() { Set> subclasses = new HashSet<>(); try { - Reflections reflections = new Reflections("org.apache.hadoop"); + Reflections reflections = new Reflections("org.apache"); subclasses.addAll(reflections.getSubTypesOf(DBDefinition.class)); - subclasses.remove(Class.forName("org.apache.hadoop.hdds.utils.db.DBDefinition$WithMap")); - subclasses.remove(Class.forName("org.apache.hadoop.hdds.utils.db.DBDefinition$WithMapInterface")); - subclasses.remove(Class.forName( - "org.apache.hadoop.ozone.container.metadata.AbstractDatanodeDBDefinition")); + subclasses.remove(DBDefinition.WithMap.class); + subclasses.remove(DBDefinition.WithMapInterface.class); + subclasses.remove(AbstractDatanodeDBDefinition.class); } catch (Exception e) { fail("Error while finding subclasses: " + e.getMessage()); }