Skip to content

OAK-11967: RDBDocumentStore: code cleanup #2555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
reschke wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open

OAK-11967: RDBDocumentStore: code cleanup #2555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff4d99d
OAK-11967: RDBDocumentStore: code cleanup
reschke Oct 2, 2025
42818c0
OAK-11967: RDBDocumentStore: code cleanup
reschke Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 32 additions & 41 deletions ...e-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ public RDBBlobStore(@NotNull DataSource ds) {

@Override
public void close() {
String dropped = "";
StringBuilder dropped = new StringBuilder();
if (!this.tablesToBeDropped.isEmpty()) {
LOG.debug("attempting to drop: " + this.tablesToBeDropped);
LOG.debug("attempting to drop: {}", this.tablesToBeDropped);
for (String tname : this.tablesToBeDropped) {
Connection con = null;
try {
Expand All @@ -106,28 +106,26 @@ public void close() {
stmt.execute("drop table " + tname);
stmt.close();
con.commit();
dropped += tname + " ";
dropped.append(tname).append(" ");
} catch (SQLException ex) {
LOG.debug("attempting to drop: " + tname, ex);
LOG.debug("attempting to drop: {}", tname, ex);
} finally {
closeStatement(stmt);
}
} catch (SQLException ex) {
LOG.debug("attempting to drop: " + tname, ex);
LOG.debug("attempting to drop: {}", tname, ex);
} finally {
this.ch.closeConnection(con);
}
}
dropped = dropped.trim();
dropped = new StringBuilder(dropped.toString().trim());
}

this.ch.close();

LOG.info("RDBBlobStore (" + getModuleVersion() + ") closed"
+ (dropped.isEmpty() ? "" : " (tables dropped: " + dropped + ")"));
LOG.info("RDBBlobStore ({}) closed{}", getModuleVersion(), (dropped.length() == 0) ? "" : " (tables dropped: " + dropped + ")");
}

@SuppressWarnings("deprecation")
@Override
protected void finalize() throws Throwable {
if (!this.ch.isClosed() && this.callStack != null) {
Expand Down Expand Up @@ -160,12 +158,12 @@ protected void finalize() throws Throwable {
// from options
protected String tnData;
protected String tnMeta;
private Set<String> tablesToBeDropped = new HashSet<String>();
private final Set<String> tablesToBeDropped = new HashSet<>();
private boolean readOnly;

private void initialize(DataSource ds, DocumentNodeStoreBuilder<?> builder, RDBOptions options) throws Exception {

this.readOnly = builder == null ? false : builder.getReadOnlyMode();
this.readOnly = builder != null && builder.getReadOnlyMode();

this.tnData = RDBJDBCTools.createTableName(options.getTablePrefix(), "DATASTORE_DATA");
this.tnMeta = RDBJDBCTools.createTableName(options.getTablePrefix(), "DATASTORE_META");
Expand All @@ -176,10 +174,7 @@ private void initialize(DataSource ds, DocumentNodeStoreBuilder<?> builder, RDBO
int isolation = con.getTransactionIsolation();
String isolationDiags = RDBJDBCTools.isolationLevelToString(isolation);
if (isolation != Connection.TRANSACTION_READ_COMMITTED) {
LOG.info("Detected transaction isolation level " + isolationDiags + " is "
+ (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));
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

DatabaseMetaData md = con.getMetaData();
Expand All @@ -195,8 +190,8 @@ private void initialize(DataSource ds, DocumentNodeStoreBuilder<?> builder, RDBO
md.getDriverMinorVersion()).replaceAll("[\r\n\t]", " ").trim();
String dbUrl = md.getURL();

List<String> tablesCreated = new ArrayList<String>();
List<String> tablesPresent = new ArrayList<String>();
List<String> tablesCreated = new ArrayList<>();
List<String> tablesPresent = new ArrayList<>();
Map<String, String> tableInfo = new HashMap<>();

Statement createStatement = null;
Expand All @@ -223,21 +218,21 @@ private void initialize(DataSource ds, DocumentNodeStoreBuilder<?> builder, RDBO
// table does not appear to exist
con.rollback();

LOG.debug("trying to read from '" + tableName + "'", ex);
LOG.debug("trying to read from '{}'", tableName, ex);
if (this.readOnly) {
throw new SQLException("Would like to create table '" + tableName
+ "', but RDBBlobStore has been initialized in 'readonly' mode");
}

createStatement = con.createStatement();

String ct;
if (this.tnMeta.equals(tableName)) {
String ct = db.getMetaTableCreationStatement(tableName);
createStatement.execute(ct);
ct = db.getMetaTableCreationStatement(tableName);
} else {
String ct = db.getDataTableCreationStatement(tableName);
createStatement.execute(ct);
ct = db.getDataTableCreationStatement(tableName);
}
createStatement.execute(ct);

createStatement.close();
createStatement = null;
Expand Down Expand Up @@ -265,15 +260,12 @@ private void initialize(DataSource ds, DocumentNodeStoreBuilder<?> builder, RDBO

Map<String, String> diag = db.getAdditionalDiagnostics(this.ch, this.tnData);

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);
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so above

if (!tablesPresent.isEmpty()) {
LOG.info("Tables present upon startup: " + tablesPresent);
LOG.info("Tables present upon startup: {}", tablesPresent);
}
if (!tablesCreated.isEmpty()) {
LOG.info("Tables created upon startup: " + tablesCreated
+ (options.isDropTablesOnClose() ? " (will be dropped on exit)" : ""));
LOG.info("Tables created upon startup: {}{}", tablesCreated, options.isDropTablesOnClose() ? " (will be dropped on exit)" : "");
}

String moreDiags = db.evaluateDiagnostics(diag);
Expand Down Expand Up @@ -341,7 +333,7 @@ private void storeBlockInDatabase(byte[] digest, int level, byte[] data) throws
}
} catch (SQLException ex) {
this.ch.rollbackConnection(con);
// the insert failed although it should have succeeded; see whether the blob already exists
// the insert failed, although it should have succeeded; see whether the blob already exists
prep = con.prepareStatement("select DATA from " + this.tnData + " where ID = ?");
ResultSet rs = null;
byte[] dbdata = null;
Expand Down Expand Up @@ -370,7 +362,7 @@ else if (!Arrays.equals(data, dbdata)) {
}
else {
// just recover
LOG.info("recovered from DB inconsistency for id " + id + ": meta record was missing (impact will be minor performance degradation)");
LOG.info("recovered from DB inconsistency for id {}: meta record was missing (impact will be minor performance degradation)", id);
}
}
try {
Expand All @@ -389,7 +381,7 @@ else if (!Arrays.equals(data, dbdata)) {
}
} catch (SQLException e) {
// already exists - ok
LOG.debug("inserting meta record for id " + id, e);
LOG.debug("inserting meta record for id {}", id, e);
}
}
} finally {
Expand Down Expand Up @@ -510,7 +502,7 @@ private int sweepFromDatabase() throws SQLException {
prepCheck = con.prepareStatement("select ID from " + this.tnMeta + " where LASTMOD < ?");
prepCheck.setLong(1, minLastModified);
rs = prepCheck.executeQuery();
ArrayList<String> ids = new ArrayList<String>();
ArrayList<String> ids = new ArrayList<>();
while (rs.next()) {
ids.add(rs.getString(1));
}
Expand Down Expand Up @@ -569,8 +561,7 @@ public long countDeleteChunks(List<String> chunkIds, long maxLastModifiedTime) t
metaStatement.append(" and LASTMOD <= ?");
// delete if there is NO entry where the last modified of
// 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 > ?)");
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

prepMeta = con.prepareStatement(metaStatement.toString());
Expand Down Expand Up @@ -619,12 +610,12 @@ public Iterator<String> getAllChunkIds(long maxLastModifiedTime) throws Exceptio
*/
private static class ChunkIdIterator extends AbstractIterator<String> {

private long maxLastModifiedTime;
private RDBConnectionHandler ch;
private static int BATCHSIZE = 1024 * 64;
private List<String> results = new LinkedList<String>();
private final long maxLastModifiedTime;
private final RDBConnectionHandler ch;
private static final int BATCHSIZE = 1024 * 64;
private final List<String> results = new LinkedList<>();
private String lastId = null;
private String metaTable;
private final String metaTable;

public ChunkIdIterator(RDBConnectionHandler ch, long maxLastModifiedTime, String metaTable) {
this.maxLastModifiedTime = maxLastModifiedTime;
Expand All @@ -647,8 +638,8 @@ protected String computeNext() {
}

private boolean refill() {
StringBuffer query = new StringBuffer();
query.append("select ID from " + metaTable);
StringBuilder query = new StringBuilder();
query.append("select ID from ").append(metaTable);
if (maxLastModifiedTime > 0) {
query.append(" where LASTMOD <= ?");
if (lastId != null) {
Expand Down
7 changes: 3 additions & 4 deletions ...document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStoreDB.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,11 @@ public String getMetaTableCreationStatement(String tableName) {

protected RDBCommonVendorSpecificCode vendorCode = RDBCommonVendorSpecificCode.DEFAULT;

private RDBBlobStoreDB(String description) {
RDBBlobStoreDB(String description) {
this.description = description;
this.vendorCode = RDBCommonVendorSpecificCode.DEFAULT;
}

private RDBBlobStoreDB(String description, RDBCommonVendorSpecificCode vendorCode) {
RDBBlobStoreDB(String description, RDBCommonVendorSpecificCode vendorCode) {
this.description = description;
this.vendorCode = vendorCode;
}
Expand Down Expand Up @@ -187,7 +186,7 @@ public static RDBBlobStoreDB getValue(String desc) {
}
}

LOG.error("DB type " + desc + " unknown, trying default settings");
LOG.error("DB type {} unknown, trying default settings", desc);
DEFAULT.description = desc + " - using default settings";
return DEFAULT;
}
Expand Down
12 changes: 6 additions & 6 deletions ...main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBCommonVendorSpecificCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public enum RDBCommonVendorSpecificCode {

DB2() {
@Override
public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
public @NotNull Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
Connection con = null;
PreparedStatement stmt = null;
ResultSet rs = null;
Expand Down Expand Up @@ -85,7 +85,7 @@ public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, Str

MSSQL() {
@Override
public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
public @NotNull Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
Connection con = null;
PreparedStatement stmt = null;
ResultSet rs = null;
Expand Down Expand Up @@ -116,7 +116,7 @@ public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, Str

MYSQL() {
@Override
public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
public @NotNull Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
Connection con = null;
PreparedStatement stmt = null;
ResultSet rs = null;
Expand Down Expand Up @@ -153,7 +153,7 @@ public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, Str

ORACLE() {
@Override
public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
public @NotNull Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
Connection con = null;
Statement stmt = null;
ResultSet rs = null;
Expand Down Expand Up @@ -181,7 +181,7 @@ public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, Str

POSTGRES() {
@Override
public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
public @NotNull Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, String tableName) {
Connection con = null;
PreparedStatement stmt = null;
ResultSet rs = null;
Expand Down Expand Up @@ -212,7 +212,7 @@ public Map<String, String> getAdditionalDiagnostics(RDBConnectionHandler ch, Str

protected String description;

private RDBCommonVendorSpecificCode() {
RDBCommonVendorSpecificCode() {
}

@NotNull
Expand Down
19 changes: 8 additions & 11 deletions ...nt/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class RDBConnectionHandler implements Closeable {
* This becomes a problem when the pool implemented by the {@link DataSource} re-uses the connection, and may
* affect subsequent users of that connection. This system property allows to enable a check to be done upon
* {@link #closeConnection(Connection)} so that problems can be detected early rather than late.
* See also https://issues.apache.org/jira/browse/OAK-2337.
* See also <a href="https://issues.apache.org/jira/browse/OAK-2337">...</a>.
*/
private static final boolean CHECKCONNECTIONONCLOSE = SystemPropertySupplier
.create("org.apache.jackrabbit.oak.plugins.document.rdb.RDBConnectionHandler.CHECKCONNECTIONONCLOSE", Boolean.FALSE)
Expand Down Expand Up @@ -155,7 +155,7 @@ private Connection getConnection() throws IllegalStateException, SQLException {
if (LOG.isDebugEnabled()) {
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"));
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"));
}

}
}
return c;
Expand All @@ -176,8 +176,7 @@ private void setReadOnly(Connection c, boolean ro) throws SQLException {
c.setReadOnly(true);
this.setReadOnlyThrows = Boolean.FALSE;
} catch (SQLException ex) {
LOG.error("Connection class " + c.getClass()
+ " erroneously throws SQLException on setReadOnly(true); not trying again");
LOG.error("Connection class {} erroneously throws SQLException on setReadOnly(true); not trying again", c.getClass());
this.setReadOnlyThrows = Boolean.TRUE;
}
} else if (!this.setReadOnlyThrows) {
Expand All @@ -190,8 +189,7 @@ private void setReadOnly(Connection c, boolean ro) throws SQLException {
c.setReadOnly(false);
this.setReadWriteThrows = Boolean.FALSE;
} 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());
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

this.setReadWriteThrows = Boolean.TRUE;
}
} else if (!this.setReadWriteThrows) {
Expand Down Expand Up @@ -222,7 +220,7 @@ public String dump(long now) {
}

// map holding references to currently open connections
private ConcurrentMap<WeakReference<Connection>, ConnectionHolder> connectionMap = new ConcurrentHashMap<>();
private final ConcurrentMap<WeakReference<Connection>, ConnectionHolder> connectionMap = new ConcurrentHashMap<>();

// time in millis for a connection in the map to be logged as "old"; note
// that this is meant to catch both connection leaks and long-running
Expand Down Expand Up @@ -255,16 +253,15 @@ private void dumpConnectionMap(long ts) {
}
}
if (cnt > 0) {
LOG.trace(cnt + " connections with age >= " + LOGTHRESHOLD + "ms active while obtaining new connection: "
+ sb.toString());
LOG.trace("{} connections with age >= " + LOGTHRESHOLD + "ms active while obtaining new connection: {}", cnt, sb);
Copy link
Contributor

@joerghoh joerghoh Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LOG.trace("{} connections with age >= " + LOGTHRESHOLD + "ms active while obtaining new connection: {}", cnt, sb);
LOG.trace("{} connections with age >= {}ms active while obtaining new connection: {}", cnt, LOGTHRESHOLD, sb);

}
}
}
}

private void remember(Connection c) {
if (LOG.isTraceEnabled()) {
connectionMap.put(new WeakReference<Connection>(c), new ConnectionHolder());
connectionMap.put(new WeakReference<>(c), new ConnectionHolder());
}
}

Expand All @@ -288,7 +285,7 @@ private static String getCaller(StackTraceElement[] elements) {
}
sb.append('.').append(e.getMethodName()).append('(').append(loc).append(')');
} else {
sb.append(e.toString());
sb.append(e);
}
prevClass = e.getClassName();
}
Expand Down
Loading
Loading