[GitHub] [flink] masteryhx commented on a diff in pull request #22458: [FLINK-31743][statebackend/rocksdb] disable rocksdb log relocating wh…

["masteryhx (via GitHub)" <gi...@apache.org>]

masteryhx commented on code in PR #22458:
URL: https://github.com/apache/flink/pull/22458#discussion_r1176146596


##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java:
##########
@@ -303,6 +310,12 @@ private <T> T internalGetOption(ConfigOption<T> option) {
 
     @SuppressWarnings("ConstantConditions")

Review Comment:
   This SuppressWarnings could be removed ?
   Or this method could be removed ?



##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java:
##########
@@ -59,6 +59,9 @@
 public final class RocksDBResourceContainer implements AutoCloseable {
     private static final Logger LOG = LoggerFactory.getLogger(RocksDBResourceContainer.class);
 
+    // the filename length limit is 255 on most operating systems
+    private static final int INSTANCE_PATH_LENGTH_LIMIT = 255 - "_LOG".length();

Review Comment:
   Just a minor question.
   why we need to reduce the length of "_LOG"?



##########
flink-state-backends/flink-statebackend-rocksdb/src/main/java/org/apache/flink/contrib/streaming/state/RocksDBResourceContainer.java:
##########
@@ -313,8 +326,17 @@ private DBOptions setDBOptionsFromConfigurableOptions(DBOptions currentOptions)
         currentOptions.setInfoLogLevel(internalGetOption(RocksDBConfigurableOptions.LOG_LEVEL));
 
         String logDir = internalGetOption(RocksDBConfigurableOptions.LOG_DIR);
+
         if (logDir == null || logDir.isEmpty()) {
-            relocateDefaultDbLogDir(currentOptions);
+            if (instanceRocksDBPath == null
+                    || instanceRocksDBPath.getAbsolutePath().length()
+                            <= INSTANCE_PATH_LENGTH_LIMIT) {
+                relocateDefaultDbLogDir(currentOptions);
+            } else {
+                // disable log relocate when instance path length exceeds limit to prevent rocksdb
+                // log file creation failure, details in FLINK-31743
+                LOG.warn("RocksDB base path length exceeds limit, disable log relocate.");

Review Comment:
   We could also log instanceRocksDBPath in the warn info ?



##########
flink-state-backends/flink-statebackend-rocksdb/src/test/java/org/apache/flink/contrib/streaming/state/RocksDBStateBackendConfigTest.java:
##########
@@ -106,6 +106,12 @@ public void testDefaultDbLogDir() throws Exception {
                     RocksDBConfigurableOptions.LOG_LEVEL.defaultValue(),
                     container.getDbOptions().infoLogLevel());
             assertEquals(logFile.getParent(), container.getDbOptions().dbLogDir());
+
+            String longInstancePath = tempFolder.newFolder().getAbsolutePath();
+            while (longInstancePath.length() < 255) {
+                longInstancePath += "/append-for-long-path";

Review Comment:
   I'd prefer to use StringBuilder even if compiler could have optimization on '+='.
   



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org