OAK-11967: RDBDocumentStore: code cleanup by reschke · Pull Request #2555 · apache/jackrabbit-oak

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAK-11967: RDBDocumentStore: code cleanup#2555

OAK-11967: RDBDocumentStore: code cleanup#2555
reschke wants to merge 2 commits intotrunkfrom
OAK-11967

reschke commented Oct 2, 2025

Uh oh!

sonarqubecloud bot commented Oct 7, 2025

Uh oh!

joerghoh left a comment

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

joerghoh Oct 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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