Copilot commented on code in PR #3024: URL: https://github.com/apache/hugegraph/pull/3024#discussion_r3310217210
########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/AbstractRocksDBProvider.java: ########## @@ -0,0 +1,176 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +/** + * Abstract base class for RocksDB providers that provides common utility methods. + * This class focuses only on the core abstraction of RocksDB open/close operations + * without complex configuration management. + */ +public abstract class AbstractRocksDBProvider implements RocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractRocksDBProvider.class); + + @Override + public final RocksDB openRocksDB(Options options, String dataPath) throws RocksDBException { + LOG.debug("Opening RocksDB with provider: {} at path: {}", getProviderName(), dataPath); + + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = doOpenRocksDB(options, dataPath); + + return rocksDB; + } catch (Exception e) { + LOG.error("Failed to open RocksDB with provider: {} at path: {}", + getProviderName(), dataPath, e); + throw e; + } Review Comment: `catch (Exception e) { ... throw e; }` won’t compile here because this method only declares `throws RocksDBException`, but `e` is a generic checked `Exception`. Catch `RocksDBException` (and possibly `RuntimeException`) explicitly, or wrap unexpected exceptions into a new `RocksDBException`/`RuntimeException` before rethrowing. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/AbstractRocksDBProvider.java: ########## @@ -0,0 +1,176 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +/** + * Abstract base class for RocksDB providers that provides common utility methods. + * This class focuses only on the core abstraction of RocksDB open/close operations + * without complex configuration management. + */ +public abstract class AbstractRocksDBProvider implements RocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractRocksDBProvider.class); + + @Override + public final RocksDB openRocksDB(Options options, String dataPath) throws RocksDBException { + LOG.debug("Opening RocksDB with provider: {} at path: {}", getProviderName(), dataPath); + + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = doOpenRocksDB(options, dataPath); + + return rocksDB; + } catch (Exception e) { + LOG.error("Failed to open RocksDB with provider: {} at path: {}", + getProviderName(), dataPath, e); + throw e; + } + } + + @Override + public final RocksDB openRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles) throws RocksDBException { + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = doOpenRocksDB(dbOptions, dataPath, cfDescriptors, cfHandles); + + return rocksDB; + } catch (Exception e) { + LOG.error("Failed to open RocksDB with column families using provider: {} at path: {}", + getProviderName(), dataPath, e); + throw e; + } Review Comment: Same issue as above: `catch (Exception e) { ... throw e; }` won’t compile in a method that only declares `throws RocksDBException`. Narrow the catch to `RocksDBException` or wrap other checked exceptions before rethrowing. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/StandardRocksDBProvider.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +/** + * Standard RocksDB provider implementation that uses the standard RocksDB library. + * This provider handles the traditional RocksDB opening and configuration logic. + */ +public class StandardRocksDBProvider extends AbstractRocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(StandardRocksDBProvider.class); + + private static final String PROVIDER_NAME = "standard"; + private static final int PROVIDER_PRIORITY = 100; // Lower priority than ToplingDB + + @Override + public String getProviderName() { + return PROVIDER_NAME; + } + + @Override + public int getPriority() { + return PROVIDER_PRIORITY; + } + + @Override + public boolean isAvailable() { + try { + // Check if standard RocksDB is available + RocksDB.loadLibrary(); + return true; + } catch (Exception e) { + LOG.warn("Standard RocksDB is not available: {}", e.getMessage()); + return false; + } + } + + @Override + protected RocksDB doOpenRocksDB(Options options, String dataPath) throws RocksDBException { + try { + return RocksDB.open(options, dataPath); + } catch (RocksDBException e) { + throw e; + } + } + + @Override + public RocksDB openRocksDB(Options options, String dataPath, String optionPath, + Boolean openHttp) throws RocksDBException { + return doOpenRocksDB(options, dataPath, optionPath, openHttp); + } + + @Override + protected RocksDB doOpenRocksDB(Options options, String dataPath, String optionPath, + Boolean openHttp) throws RocksDBException { + LOG.debug("Opening standard RocksDB with Options and extended parameters at path: {}", + dataPath); + + // Log warnings for unsupported parameters + if (optionPath != null) { + LOG.warn("Standard RocksDB does not support optionPath parameter, ignoring: {}", + optionPath); + } + if (openHttp != null && openHttp) { + LOG.warn("Standard RocksDB does not support HTTP server, ignoring openHttp parameter"); + } + + // Use standard opening + return RocksDB.open(options, dataPath); + } + + @Override + protected RocksDB doOpenRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles) throws RocksDBException { + try { + return RocksDB.open(dbOptions, dataPath, cfDescriptors, cfHandles); + } catch (RocksDBException e) { + throw e; + } + } + + @Override + protected RocksDB doOpenRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles, + String optionPath, Boolean openHttp) throws RocksDBException { + // Standard RocksDB doesn't support optionPath and openHttp parameters + // Log a warning if these parameters are provided + if (optionPath != null && !optionPath.isEmpty()) { + LOG.warn("Standard RocksDB provider does not support optionPath parameter: {}", + optionPath); + } + if (openHttp != null && openHttp) { + LOG.warn("Standard RocksDB provider does not support openHttp parameter"); + } + + // Fallback to standard column family opening + return doOpenRocksDB(dbOptions, dataPath, cfDescriptors, cfHandles); + } + + @Override + public void initialize() { + try { + // Load RocksDB native library + RocksDB.loadLibrary(); + LOG.debug("Standard RocksDB library loaded successfully"); + } catch (Exception e) { + LOG.error("Failed to load standard RocksDB library", e); + throw new RuntimeException("Failed to initialize standard RocksDB provider", e); + } Review Comment: Same `UnsatisfiedLinkError` risk as in `isAvailable()`: `RocksDB.loadLibrary()` can throw `Error` types. Catching only `Exception` may terminate initialization instead of failing provider selection gracefully. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/AbstractRocksDBProvider.java: ########## @@ -0,0 +1,176 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +/** + * Abstract base class for RocksDB providers that provides common utility methods. + * This class focuses only on the core abstraction of RocksDB open/close operations + * without complex configuration management. + */ +public abstract class AbstractRocksDBProvider implements RocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(AbstractRocksDBProvider.class); + + @Override + public final RocksDB openRocksDB(Options options, String dataPath) throws RocksDBException { + LOG.debug("Opening RocksDB with provider: {} at path: {}", getProviderName(), dataPath); + + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = doOpenRocksDB(options, dataPath); + + return rocksDB; + } catch (Exception e) { + LOG.error("Failed to open RocksDB with provider: {} at path: {}", + getProviderName(), dataPath, e); + throw e; + } + } + + @Override + public final RocksDB openRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles) throws RocksDBException { + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = doOpenRocksDB(dbOptions, dataPath, cfDescriptors, cfHandles); + + return rocksDB; + } catch (Exception e) { + LOG.error("Failed to open RocksDB with column families using provider: {} at path: {}", + getProviderName(), dataPath, e); + throw e; + } + } + + @Override + public final RocksDB openRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles, + String optionPath, Boolean openHttp) throws RocksDBException { + + LOG.debug("Opening RocksDB with extended parameters using provider: {} at path: {}", + getProviderName(), dataPath); + // Initialize provider if needed + initialize(); + + try { + // Delegate to provider-specific implementation + RocksDB rocksDB = + doOpenRocksDB(dbOptions, dataPath, cfDescriptors, cfHandles, optionPath, + openHttp); + return rocksDB; + } catch (Exception e) { + throw e; + } Review Comment: `catch (Exception e) { throw e; }` won’t compile here for the same reason: the method signature only allows `RocksDBException`. Either catch/rethrow `RocksDBException` explicitly or wrap unexpected checked exceptions. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/StandardRocksDBProvider.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.List; + +/** + * Standard RocksDB provider implementation that uses the standard RocksDB library. + * This provider handles the traditional RocksDB opening and configuration logic. + */ +public class StandardRocksDBProvider extends AbstractRocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(StandardRocksDBProvider.class); + + private static final String PROVIDER_NAME = "standard"; + private static final int PROVIDER_PRIORITY = 100; // Lower priority than ToplingDB + + @Override + public String getProviderName() { + return PROVIDER_NAME; + } + + @Override + public int getPriority() { + return PROVIDER_PRIORITY; + } + + @Override + public boolean isAvailable() { + try { + // Check if standard RocksDB is available + RocksDB.loadLibrary(); + return true; + } catch (Exception e) { + LOG.warn("Standard RocksDB is not available: {}", e.getMessage()); Review Comment: `RocksDB.loadLibrary()` can fail with `UnsatisfiedLinkError` (an `Error`, not an `Exception`). Catching only `Exception` here can crash provider discovery on hosts without native libs. Consider catching `Throwable` (or at least `LinkageError`/`UnsatisfiedLinkError`) and returning `false` with a log message. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/RocksDBProviderLoader.java: ########## @@ -0,0 +1,290 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.ColumnFamilyDescriptor; +import org.rocksdb.ColumnFamilyHandle; +import org.rocksdb.DBOptions; +import org.rocksdb.Options; +import org.rocksdb.RocksDB; +import org.rocksdb.RocksDBException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; + +/** + * RocksDB Provider SPI Loader that manages the loading and selection + * of RocksDB providers using Java's ServiceLoader mechanism. + */ +public class RocksDBProviderLoader { + + private static final Logger LOG = LoggerFactory.getLogger(RocksDBProviderLoader.class); + + private static final RocksDBProviderLoader INSTANCE = new RocksDBProviderLoader(); + + private final Map<String, RocksDBProvider> providerCache = new ConcurrentHashMap<>(); + private volatile boolean loaded = false; + private volatile RocksDBProvider bestProvider = null; + + private RocksDBProviderLoader() { + // Private constructor for singleton + } + + public static RocksDBProviderLoader getInstance() { + return INSTANCE; + } + + /** + * Load all available RocksDB providers using SPI + */ + public synchronized void loadProviders() { + if (loaded) { + return; + } + + LOG.info("Loading RocksDB providers via SPI..."); + + ServiceLoader<RocksDBProvider> serviceLoader = ServiceLoader.load(RocksDBProvider.class); + + for (RocksDBProvider provider : serviceLoader) { + try { + if (provider.isAvailable()) { + providerCache.put(provider.getProviderName(), provider); + LOG.info("Loaded RocksDB provider: {} (priority: {})", + provider.getProviderName(), provider.getPriority()); + } else { + LOG.warn("RocksDB provider {} is not available in current environment", + provider.getProviderName()); + } + } catch (Exception e) { + LOG.error("Failed to load RocksDB provider: {}", provider.getClass().getName(), e); + } + } + + if (providerCache.isEmpty()) { + LOG.warn( + "No RocksDB providers found! Make sure providers are properly registered in " + + "META-INF/services"); + } else { + LOG.info("Successfully loaded {} RocksDB provider(s): {}", + providerCache.size(), providerCache.keySet()); + } + + loaded = true; + } + + /** + * Get a specific provider by name + * + * @param providerName provider name + * @return RocksDB provider or null if not found + */ + public RocksDBProvider getProvider(String providerName) { + if (!loaded) { + loadProviders(); + } + + return providerCache.get(providerName); + } + + /** + * Get the best available provider based on priority + * + * @return best available RocksDB provider + */ + public RocksDBProvider getBestProvider() { + if (!loaded) { + loadProviders(); + } + + RocksDBProvider cached = this.bestProvider; + if (cached != null) { + return cached; + } + + if (providerCache.isEmpty()) { + throw new RuntimeException("No RocksDB providers available"); + } + + synchronized (this) { + if (this.bestProvider != null) { + return this.bestProvider; + } + + // Find provider with highest priority + RocksDBProvider selected = null; + int highestPriority = Integer.MIN_VALUE; + + for (RocksDBProvider provider : providerCache.values()) { + if (provider.isAvailable() && provider.getPriority() > highestPriority) { + selected = provider; + highestPriority = provider.getPriority(); + } + } Review Comment: `getBestProvider()` calls `provider.isAvailable()` without guarding against `Error`s. If native loading fails with `UnsatisfiedLinkError`, this can crash DB open/close. Consider wrapping the `isAvailable()` call in try/catch (including `Throwable`) and treating failures as “not available”. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/RocksDBProviderLoader.java: ########## @@ -0,0 +1,290 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.ColumnFamilyDescriptor; +import org.rocksdb.ColumnFamilyHandle; +import org.rocksdb.DBOptions; +import org.rocksdb.Options; +import org.rocksdb.RocksDB; +import org.rocksdb.RocksDBException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; + +/** + * RocksDB Provider SPI Loader that manages the loading and selection + * of RocksDB providers using Java's ServiceLoader mechanism. + */ +public class RocksDBProviderLoader { + + private static final Logger LOG = LoggerFactory.getLogger(RocksDBProviderLoader.class); + + private static final RocksDBProviderLoader INSTANCE = new RocksDBProviderLoader(); + + private final Map<String, RocksDBProvider> providerCache = new ConcurrentHashMap<>(); + private volatile boolean loaded = false; + private volatile RocksDBProvider bestProvider = null; + + private RocksDBProviderLoader() { + // Private constructor for singleton + } + + public static RocksDBProviderLoader getInstance() { + return INSTANCE; + } + + /** + * Load all available RocksDB providers using SPI + */ + public synchronized void loadProviders() { + if (loaded) { + return; + } + + LOG.info("Loading RocksDB providers via SPI..."); + + ServiceLoader<RocksDBProvider> serviceLoader = ServiceLoader.load(RocksDBProvider.class); + + for (RocksDBProvider provider : serviceLoader) { + try { + if (provider.isAvailable()) { + providerCache.put(provider.getProviderName(), provider); + LOG.info("Loaded RocksDB provider: {} (priority: {})", + provider.getProviderName(), provider.getPriority()); + } else { + LOG.warn("RocksDB provider {} is not available in current environment", + provider.getProviderName()); + } + } catch (Exception e) { Review Comment: `provider.isAvailable()` can throw `UnsatisfiedLinkError`/other `Error`s (see `StandardRocksDBProvider.isAvailable()`), but this loop only catches `Exception`. An `Error` would abort provider loading and potentially crash the process. Consider catching `Throwable` around provider instantiation/availability checks so a single broken provider can be skipped safely. ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/config/PDConfig.java: ########## @@ -58,6 +58,11 @@ public class PDConfig { @Value("${grpc.host}") private String host; + @Value("${rocksdb.option-path: ''}") Review Comment: With `@Value("${rocksdb.option-path: ''}")`, the default becomes the literal string `''` (including quotes/spaces) rather than an empty string. That makes `optionPath` non-empty by default and will trigger Topling/RocksDB option-path validation and warning logs even when users didn’t configure it. Consider using an actual empty default like `${rocksdb.option-path:}` (or `${rocksdb.option-path:}` + `.trim()` when consuming). ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/ToplingRocksDBProvider.java: ########## @@ -0,0 +1,614 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.*; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Locale; +import java.util.List; +import java.util.concurrent.ConcurrentHashMap; +import java.util.Map; +import java.util.regex.Pattern; + +import net.minidev.json.JSONObject; + +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.BaseConstructor; +import org.yaml.snakeyaml.constructor.SafeConstructor; +import org.yaml.snakeyaml.LoaderOptions; + +/** + * ToplingRocksDBProvider provides ToplingDB-specific RocksDB functionality. + * This provider supports advanced ToplingDB features including: + * - YAML-based configuration via optionPath + * - HTTP server for monitoring and management + * - SidePluginRepo integration for enhanced performance + */ +public class ToplingRocksDBProvider extends AbstractRocksDBProvider { + + private static final Logger LOG = LoggerFactory.getLogger(ToplingRocksDBProvider.class); + + private static final String PROVIDER_NAME = "topling"; + private static final int PROVIDER_PRIORITY = 200; // Higher priority than standard + private static final String SIDE_PLUGIN_REPO_CLASS = "org.rocksdb.SidePluginRepo"; + + // Validation constants migrated from RocksDBOptions + private static final Pattern SAFE_PATH_PATTERN = + Pattern.compile("^[a-zA-Z0-9/_.-]+\\.ya?ml$"); + private static final String ALLOWED_CONFIG_DIR = "./conf/"; + private static final long MAX_CONFIG_FILE_SIZE = 1024 * 1024 * 10; // 10 MB + + // Store repo objects for proper cleanup + private final Map<RocksDB, Object> rocksDBToRepoMap = new ConcurrentHashMap<>(); + + @Override + public String getProviderName() { + return PROVIDER_NAME; + } + + @Override + public int getPriority() { + return PROVIDER_PRIORITY; + } + + @Override + public boolean isAvailable() { + try { + // Check if SidePluginRepo class is available + Class.forName(SIDE_PLUGIN_REPO_CLASS); + LOG.info("ToplingDB SidePluginRepo found, ToplingRocksDBProvider is available"); + return true; + } catch (ClassNotFoundException e) { + LOG.debug( + "ToplingDB SidePluginRepo not found, ToplingRocksDBProvider is not available:" + + " {}", + e.getMessage()); + return false; + } + } + + @Override + protected RocksDB doOpenRocksDB(Options options, String dataPath) throws RocksDBException { + LOG.info("Opening RocksDB with Options at path: {}", dataPath); + + // For simple Options-based opening without optionPath, use standard RocksDB.open + return RocksDB.open(options, dataPath); + } + + @Override + public RocksDB openRocksDB(Options options, String dataPath, String optionPath, + Boolean openHttp) throws RocksDBException { + initialize(); + return doOpenRocksDB(options, dataPath, optionPath, openHttp); + } + + @Override + protected RocksDB doOpenRocksDB(Options options, String dataPath, String optionPath, + Boolean openHttp) throws RocksDBException { + // Check if we should use ToplingDB features + boolean useTopling = validateConfiguration(optionPath); + + if (useTopling) { + return openWithToplingFeatures(options, dataPath, optionPath, openHttp); + } else { + logStandardFallback(optionPath); + return RocksDB.open(options, dataPath); + } + } + + @Override + protected RocksDB doOpenRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles) throws RocksDBException { + LOG.info("Opening RocksDB with DBOptions and column families at path: {}", dataPath); + + // For column family opening without ToplingDB features, use standard RocksDB.open + return RocksDB.open(dbOptions, dataPath, cfDescriptors, cfHandles); + } + + @Override + protected RocksDB doOpenRocksDB(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles, + String optionPath, Boolean openHttp) throws RocksDBException { + // Check if we should use ToplingDB features + boolean useTopling = validateConfiguration(optionPath); + + if (useTopling) { + // For ToplingDB with column families, we need to use the standard RocksDB.open method + // but with ToplingDB-specific initialization through SidePluginRepo + return openWithToplingFeaturesAndCF(dbOptions, dataPath, cfDescriptors, cfHandles, + optionPath, openHttp); + } else { + logStandardFallback(optionPath); + return RocksDB.open(dbOptions, dataPath, cfDescriptors, cfHandles); + } + } + + /** + * Opens RocksDB using ToplingDB features with SidePluginRepo + */ + private RocksDB openWithToplingFeatures(Options options, String dataPath, + String optionPath, Boolean openHttp) + throws RocksDBException { + Object repo = null; + RocksDB opened = null; + boolean registered = false; + try { + // Initialize ToplingDB repo with common operations + repo = initializeToplingRepo(options, dataPath, optionPath); + + // Open database with default column families + Class<?> sidePluginRepoClass = repo.getClass(); + Method openDBMethod = sidePluginRepoClass.getMethod("openDB", String.class); + Object result = openDBMethod.invoke(repo, converseOptionsToJsonString(dataPath, null)); + + // Validate and store result before starting HTTP server + opened = validateAndStoreResult(result, repo, dataPath, 0); + registered = true; + + // Start HTTP server if needed + startHttpServerIfNeeded(repo, dataPath, openHttp, optionPath); + + return opened; + + } catch (InvocationTargetException e) { + cleanupFailedOpen(opened, repo, registered, null); + Throwable cause = e.getCause(); + if (cause instanceof RocksDBException) { + throw (RocksDBException) cause; + } + throw new RocksDBException( + "Failed to open DB with SidePluginRepo: " + cause.getMessage()); + } catch (Exception e) { + cleanupFailedOpen(opened, repo, registered, null); + throw new RocksDBException("Failed to open ToplingDB: " + e.getMessage()); + } + } + + /** + * Open RocksDB with ToplingDB features and column families support + */ + private RocksDB openWithToplingFeaturesAndCF(DBOptions dbOptions, String dataPath, + List<ColumnFamilyDescriptor> cfDescriptors, + List<ColumnFamilyHandle> cfHandles, + String optionPath, Boolean openHttp) + throws RocksDBException { + Object repo = null; + RocksDB opened = null; + boolean registered = false; + try { + // Initialize ToplingDB repo with common operations + repo = initializeToplingRepo(dbOptions, dataPath, optionPath); + + // Prepare column family names for JSON + List<String> cfNames = new java.util.ArrayList<>(); + for (ColumnFamilyDescriptor cfDescriptor : cfDescriptors) { + cfNames.add(new String(cfDescriptor.getName())); Review Comment: Column-family names are decoded using the platform default charset (`new String(byte[])`). CF names in RocksDB are byte sequences and in this codebase are typically UTF-8 encoded; using the platform default can lead to incorrect names on non-UTF8 default locales. Prefer `new String(bytes, StandardCharsets.UTF_8)`. ########## hugegraph-rocksdb-provider/pom.xml: ########## @@ -0,0 +1,99 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. + --> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <artifactId>hugegraph-rocksdb-provider</artifactId> + <version>${revision}</version> + + <name>${project.artifactId}</name> + <description> + HugeGraph RocksDB Provider - A common adapter module for RocksDB and ToplingDB integration + using SPI (Service Provider Interface) and Strategy Pattern for loose coupling and plugin-based architecture. + </description> + + <parent> + <artifactId>hugegraph</artifactId> + <groupId>org.apache.hugegraph</groupId> + <version>${revision}</version> + <relativePath>../pom.xml</relativePath> + </parent> + + <dependencies> + <!-- RocksDB dependency --> + <dependency> + <groupId>org.rocksdb</groupId> + <artifactId>rocksdbjni</artifactId> + <version>${rocksdbjni.version}</version> + </dependency> + + <!-- HugeGraph Commons for configuration and utilities --> + <dependency> + <groupId>org.apache.hugegraph</groupId> + <artifactId>hugegraph-common</artifactId> + <version>${hugegraph-commons.version}</version> + </dependency> + + <!-- Logging --> + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-api</artifactId> + <version>1.7.5</version> Review Comment: This module pins `slf4j-api` to `1.7.5`, which is significantly older than versions already present in the repo and may downgrade the resolved SLF4J API for downstream modules. The codebase explicitly excludes older `slf4j-api` versions elsewhere to avoid this (e.g. `hugegraph-server/hugegraph-hbase/pom.xml` excludes it as “redundant older version”). Consider removing the explicit version (let Maven resolve consistently) or aligning it with the project’s chosen SLF4J version. ########## hugegraph-rocksdb-provider/src/main/java/org/apache/hugegraph/rocksdb/provider/RocksDBProviderLoader.java: ########## @@ -0,0 +1,290 @@ +/* + * 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.hugegraph.rocksdb.provider; + +import org.rocksdb.ColumnFamilyDescriptor; +import org.rocksdb.ColumnFamilyHandle; +import org.rocksdb.DBOptions; +import org.rocksdb.Options; +import org.rocksdb.RocksDB; +import org.rocksdb.RocksDBException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; + +/** + * RocksDB Provider SPI Loader that manages the loading and selection + * of RocksDB providers using Java's ServiceLoader mechanism. + */ +public class RocksDBProviderLoader { + + private static final Logger LOG = LoggerFactory.getLogger(RocksDBProviderLoader.class); + + private static final RocksDBProviderLoader INSTANCE = new RocksDBProviderLoader(); + + private final Map<String, RocksDBProvider> providerCache = new ConcurrentHashMap<>(); + private volatile boolean loaded = false; + private volatile RocksDBProvider bestProvider = null; + + private RocksDBProviderLoader() { + // Private constructor for singleton + } + + public static RocksDBProviderLoader getInstance() { + return INSTANCE; + } + + /** + * Load all available RocksDB providers using SPI + */ + public synchronized void loadProviders() { + if (loaded) { + return; + } + + LOG.info("Loading RocksDB providers via SPI..."); + + ServiceLoader<RocksDBProvider> serviceLoader = ServiceLoader.load(RocksDBProvider.class); + + for (RocksDBProvider provider : serviceLoader) { + try { + if (provider.isAvailable()) { + providerCache.put(provider.getProviderName(), provider); + LOG.info("Loaded RocksDB provider: {} (priority: {})", + provider.getProviderName(), provider.getPriority()); + } else { + LOG.warn("RocksDB provider {} is not available in current environment", + provider.getProviderName()); + } + } catch (Exception e) { + LOG.error("Failed to load RocksDB provider: {}", provider.getClass().getName(), e); + } + } + + if (providerCache.isEmpty()) { + LOG.warn( + "No RocksDB providers found! Make sure providers are properly registered in " + + "META-INF/services"); + } else { + LOG.info("Successfully loaded {} RocksDB provider(s): {}", + providerCache.size(), providerCache.keySet()); + } + + loaded = true; Review Comment: There are no unit tests in this new module, but it contains non-trivial provider selection / fallback logic (SPI discovery, priority selection, open/close delegation). Adding focused tests (at least for provider selection ordering and fallback when a provider is unavailable) would help prevent regressions, especially since failures can be environment-specific. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
