OAK-11967: RDBDocumentStore: code cleanup #2555

reschke · 2025-10-02T13:56:26Z

No description provided.

sonarqubecloud · 2025-10-07T07:35:06Z

Quality Gate failed

Failed conditions
2 Security Hotspots
69.0% Coverage on New Code (required ≥ 80%)
5.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

joerghoh

a lot of minor nits, which I think the code could benefit from, mostly when it comes to readability.

+1 for the rest

joerghoh · 2025-10-08T09:12:27Z

...tore-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java

-                    + (isolation < Connection.TRANSACTION_READ_COMMITTED ? "lower" : "higher") + " than expected "
-                    + RDBJDBCTools.isolationLevelToString(Connection.TRANSACTION_READ_COMMITTED)
-                    + " - check datasource configuration");
+            LOG.info("Detected transaction isolation level {} is {} than expected {} - check datasource configuration", isolationDiags, isolation < Connection.TRANSACTION_READ_COMMITTED ? "lower" : "higher", RDBJDBCTools.isolationLevelToString(Connection.TRANSACTION_READ_COMMITTED));


mixing the the simple varag parameters with the ternary expression in a single line? I would split it up into multiple lines, so it's more readable.

joerghoh · 2025-10-08T09:13:34Z

...tore-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java

-            LOG.info("RDBBlobStore (" + getModuleVersion() + ") instantiated for database " + dbDesc + ", using driver: "
-                    + driverDesc + ", connecting to: " + dbUrl + (diag.isEmpty() ? "" : (", properties: " + diag.toString()))
-                    + ", transaction isolation level: " + isolationDiags + ", " + tableInfo);
+            LOG.info("RDBBlobStore ({}) instantiated for database {}, using driver: {}, connecting to: {}{}, transaction isolation level: {}, {}", getModuleVersion(), dbDesc, driverDesc, dbUrl, diag.isEmpty() ? "" : (", properties: " + diag), isolationDiags, tableInfo);


joerghoh · 2025-10-08T09:16:23Z

...tore-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java

                    // the meta is YOUNGER than x
-                    dataStatement.append(" and not exists(select * from " + this.tnMeta + " where " + this.tnMeta + ".ID = "
-                            + this.tnData + ".ID and LASTMOD > ?)");
+                    dataStatement.append(" and not exists(select * from ").append(this.tnMeta).append(" where ").append(this.tnMeta).append(".ID = ").append(this.tnData).append(".ID and LASTMOD > ?)");


ahhh, don't know. That StringBuilder makes it much harder to understand. Personally I would prefer a String.format() approach, but I would be fine, if you just add some newlines into this statement.

joerghoh · 2025-10-08T09:21:55Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java

            long elapsed = System.currentTimeMillis() - ts;
            if (elapsed >= 20) {
-                LOG.debug("Obtaining a new connection from " + this.ds + " took " + elapsed + "ms", new Exception("call stack"));
+                LOG.debug("Obtaining a new connection from {} took {}ms", this.ds, elapsed, new Exception("call stack"));


In case this code line is reached and the loglevel is not set to DEBUG, the exception is created (expensive!) but not used. I would guard this debug statement here, so the exception is only created when the log statement is actually printed.

Suggested change

LOG.debug("Obtaining a new connection from {} took {}ms", this.ds, elapsed, new Exception("call stack"));

if (LOG.isDebugEnabled()) {

LOG.debug("Obtaining a new connection from {} took {}ms", this.ds, elapsed, new Exception("call stack"));

}

joerghoh · 2025-10-08T09:23:21Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java

                } catch (SQLException ex) {
-                    LOG.error("Connection class " + c.getClass()
-                            + " erroneously throws SQLException on setReadOnly(false); not trying again");
+                    LOG.error("Connection class {} erroneously throws SQLException on setReadOnly(false); not trying again", c.getClass());


would it make sense to log ex.getMessage() as well to give some more diagnostics?

joerghoh · 2025-10-08T09:34:02Z

...-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java

                        if (oldDoc == null) {
                            // document was there but is now gone
-                            LOG.debug("failed to apply update because document is gone in the meantime: " + update.getId(), new Exception("call stack"));
+                            LOG.debug("failed to apply update because document is gone in the meantime: {}", update.getId(), new Exception("call stack"));


we should guard this log statement also with a check if the DEBUG log is enabled, otherweise we create the exception object without using it.

joerghoh · 2025-10-08T09:40:34Z

...ument/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBC.java

        int[] batchResults = new int[0];

-        PreparedStatement stmt = connection.prepareStatement("update " + tmd.getName()
+        String statement = "update " + tmd.getName()


that is very hard to read ... would a String.format(...) help here?

joerghoh · 2025-10-08T09:54:20Z

...tore-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBJDBCTools.java

            if (maj < dbmax || (maj == dbmax && min < dbmin)) {
-                result.append(
-                        "Unsupported " + dbname + " version: " + maj + "." + min + ", expected at least " + dbmax + "." + dbmin);
+                result.append("Unsupported ").append(dbname).append(" version: ").append(maj).append(".").append(min).append(", expected at least ").append(dbmax).append(".").append(dbmin);


Suggested change

result.append("Unsupported ").append(dbname).append(" version: ").append(maj).append(".").append(min).append(", expected at least ").append(dbmax).append(".").append(dbmin);

result.append("Unsupported ").append(dbname)

.append(" version: ").append(maj).append(".").append(min)

.append(", expected at least ").append(dbmax).append(".").append(dbmin);

joerghoh · 2025-10-08T09:55:05Z

...tore-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBJDBCTools.java

                }
-                result.append("Unsupported " + dbname + " driver version: " + md.getDriverName() + " " + maj + "." + min
-                        + ", expected at least " + drmax + "." + drmin);
+                result.append("Unsupported ").append(dbname).append(" driver version: ").append(md.getDriverName()).append(" ").append(maj).append(".").append(min).append(", expected at least ").append(drmax).append(".").append(drmin);


Suggested change

result.append("Unsupported ").append(dbname).append(" driver version: ").append(md.getDriverName()).append(" ").append(maj).append(".").append(min).append(", expected at least ").append(drmax).append(".").append(drmin);

result.append("Unsupported ").append(dbname)

.append(" driver version: ").append(md.getDriverName())

.append(" ").append(maj).append(".").append(min)

.append(", expected at least ").append(drmax).append(".").append(drmin);

joerghoh · 2025-10-08T09:57:42Z

...nt/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBMissingLastRevSeeker.java

-                return String.format("Strategy for %s set to %s (via system property %s)", RDBMissingLastRevSeeker.class.getName(),
-                        name, value);
-            }).get();
+            .loggingTo(LOG).validateWith(value -> (value == 1 || value == 2)).formatSetMessage((name, value) -> String.format("Strategy for %s set to %s (via system property %s)", RDBMissingLastRevSeeker.class.getName(),


Hard to understand ... maybe some more newlines?

reschke added 2 commits October 2, 2025 14:55

OAK-11967: RDBDocumentStore: code cleanup

ff4d99d

OAK-11967: RDBDocumentStore: code cleanup

42818c0

joerghoh reviewed Oct 8, 2025

View reviewed changes

OAK-11967: RDBDocumentStore: code cleanup #2555

Are you sure you want to change the base?

OAK-11967: RDBDocumentStore: code cleanup #2555

Uh oh!

Conversation

reschke commented Oct 2, 2025

Uh oh!

sonarqubecloud bot commented Oct 7, 2025

Quality Gate failed

Uh oh!

joerghoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants