dsmiley commented on code in PR #3412:
URL: https://github.com/apache/solr/pull/3412#discussion_r2176270962
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
Review Comment:
normalizeReceivedException looks quite confusing to me. Doesn't have
javadocs. And you are modifying it to rollback? A method called
normalizeBlahBlahBlah should do no such thing IMO; should be idempotent.
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2573,19 +2575,6 @@ public boolean checkTragicException(SolrCore solrCore) {
// failed to open an indexWriter
tragicException = e;
}
-
Review Comment:
I confess; I only understood half of that.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -307,6 +308,18 @@ public static Exception
normalizeReceivedException(SolrQueryRequest req, Excepti
if (req.getCore() != null) {
assert req.getCoreContainer() != null;
if (req.getCoreContainer().checkTragicException(req.getCore())) {
+ if (req.getCoreContainer().isZooKeeperAware()) {
+ try {
+ // If the error was something like a full file system disconnect,
this probably won't
+ // help
+ // But if it is a transient disk failure then it's worth a try
Review Comment:
join lines. Maybe a period or semicolon in-between sentences
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -1592,10 +1595,45 @@ protected void deleteBufferLogs() {
}
}
+ /**
+ * Ensures a transaction log is ready. It is either the current one, or a
new one. This method
+ * must be called with the synchronization monitor on this {@link UpdateLog}.
+ */
protected void ensureLog() {
if (tlog == null) {
- String newLogName = String.format(Locale.ROOT, LOG_FILENAME_PATTERN,
TLOG_NAME, id);
- tlog = newTransactionLog(tlogDir.resolve(newLogName), globalStrings,
false);
+ Path newLogPath;
+ int numAttempts = 0;
+ while (true) {
+ String newLogName = String.format(Locale.ROOT, LOG_FILENAME_PATTERN,
TLOG_NAME, id);
+ newLogPath = tlogDir.resolve(newLogName);
+ // We expect that the log file does not exist since id is designed to
give the index of the
+ // next transaction log to create. But in very rare cases, the log
files listed in the
+ // init() method may be stale here (file system delay?), and id may
point to an existing
+ // file.
+ if (!Files.exists(newLogPath)) {
+ break;
+ }
+ // If the "new" log file already exists, refresh the log list and
recompute id.
Review Comment:
I wonder if we should assert false instead of repeat the loop?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
