joerghoh commented on code in PR #2555:
URL: https://github.com/apache/jackrabbit-oak/pull/2555#discussion_r2413163664
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java:
##########
@@ -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));
Review Comment:
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.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java:
##########
@@ -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 > ?)");
Review Comment:
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.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java:
##########
@@ -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());
Review Comment:
would it make sense to log ``ex.getMessage()`` as well to give some more
diagnostics?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBBlobStore.java:
##########
@@ -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);
Review Comment:
so above
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java:
##########
@@ -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);
Review Comment:
```suggestion
LOG.trace("{} connections with age >= {}ms active while
obtaining new connection: {}", cnt, LOGTHRESHOLD, sb);
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -813,8 +807,7 @@ public void dispose() {
} catch (IOException ex) {
LOG.warn("Error occurred while closing nodes cache", ex);
}
- LOG.info("RDBDocumentStore (" + getModuleVersion() + ") disposed" +
getCnStats()
- + (this.droppedTables.isEmpty() ? "" : " (tables dropped: " +
this.droppedTables + ")"));
+ LOG.info("RDBDocumentStore ({}) disposed{}{}", getModuleVersion(),
getCnStats(), this.droppedTables.isEmpty() ? "" : " (tables dropped: " +
this.droppedTables + ")");
Review Comment:
as above, perhaps newlines make it more readable.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBConnectionHandler.java:
##########
@@ -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"));
Review Comment:
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.
```suggestion
if (LOG.isDebugEnabled()) {
LOG.debug("Obtaining a new connection from {} took
{}ms", this.ds, elapsed, new Exception("call stack"));
}
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1083,15 +1071,12 @@ private void initialize(DataSource ds,
DocumentNodeStoreBuilder<?> builder, RDBO
Map<String, String> diag = dbInfo.getAdditionalDiagnostics(this.ch,
this.tableMeta.get(Collection.NODES).getName());
- LOG.info("RDBDocumentStore (" + getModuleVersion() + ") instantiated
for database " + dbDesc + ", using driver: "
- + driverDesc + ", connecting to: " + dbUrl + (diag.isEmpty() ?
"" : (", properties: " + diag.toString()))
- + ", transaction isolation level: " + isolationDiags +
tableDiags);
+ LOG.info("RDBDocumentStore ({}) instantiated for database {}, using
driver: {}, connecting to: {}{}, transaction isolation level: {}{}",
getModuleVersion(), dbDesc, driverDesc, dbUrl, diag.isEmpty() ? "" : (",
properties: " + diag), isolationDiags, tableDiags);
Review Comment:
newlines?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBJDBCTools.java:
##########
@@ -229,8 +229,7 @@ protected static String versionCheck(DatabaseMetaData md,
int dbmax, int dbmin,
int min = md.getDatabaseMinorVersion();
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);
Review Comment:
```suggestion
result.append("Unsupported ").append(dbname)
.append(" version: ").append(maj).append(".").append(min)
.append(", expected at least
").append(dbmax).append(".").append(dbmin);
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1694,7 +1674,7 @@ private <T extends Document> T
internalUpdate(Collection<T> collection, UpdateOp
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"));
Review Comment:
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.
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBMissingLastRevSeeker.java:
##########
@@ -44,10 +44,8 @@ public class RDBMissingLastRevSeeker extends
MissingLastRevSeeker {
private static final int DEFAULTMODE = 2;
private static final int MODE =
SystemPropertySupplier.create(RDBMissingLastRevSeeker.class.getName() +
".MODE", DEFAULTMODE)
- .loggingTo(LOG).validateWith(value -> (value == 1 || value ==
2)).formatSetMessage((name, value) -> {
- 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(),
Review Comment:
Hard to understand ... maybe some more newlines?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1012,10 +1003,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));
Review Comment:
see above
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBJDBCTools.java:
##########
@@ -242,8 +241,7 @@ protected static String versionCheck(DatabaseMetaData md,
int dbmax, int dbmin,
if (result.length() != 0) {
result.append(", ");
}
- 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);
Review Comment:
```suggestion
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);
```
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStoreJDBC.java:
##########
@@ -330,15 +323,15 @@ public <T extends Document> Set<String> update(Connection
connection, RDBTableMe
throws SQLException {
assertNoDuplicatedIds(documents);
- Set<String> successfulUpdates = new HashSet<String>();
- List<String> updatedKeys = new ArrayList<String>();
+ Set<String> successfulUpdates = new HashSet<>();
+ List<String> updatedKeys = new ArrayList<>();
List<Long> modCounts = LOG.isTraceEnabled() ? new ArrayList<>() : null;
int[] batchResults = new int[0];
- PreparedStatement stmt = connection.prepareStatement("update " +
tmd.getName()
+ String statement = "update " + tmd.getName()
Review Comment:
that is very hard to read ... would a ``String.format(...)`` help here?
##########
oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java:
##########
@@ -1642,7 +1622,7 @@ private <T extends Document> T
internalCreateOrUpdate(Collection<T> collection,
result = internalCreateOrUpdate(collection, update,
allowCreate, checkConditions, retries - 1);
}
else {
- LOG.error("update of " + update.getId() + " failed, race
condition?");
+ LOG.error("update of {} failed, race condition?",
update.getId());
Review Comment:
why these 2 added space characters?
--
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]
