-
Notifications
You must be signed in to change notification settings - Fork 241
SYM-7915: Prevent initial loads deadlocks due to a JVM restart mid-extract #994
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.
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
base: release/3.17
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1050,9 +1050,22 @@ | |
| } | ||
| ExtractRequest extractRequest = getExtractRequestForBatch(currentBatch); | ||
| if (extractRequest != null && extractRequest.getStatus() != ExtractStatus.OK) { | ||
| sqlTemplate.update(getSql("updateExtractRequestStatus"), ExtractStatus.OK.name(), new Date(), | ||
| currentBatch.getExtractRowCount(), currentBatch.getExtractMillis(), extractRequest.getRequestId()); | ||
| checkSendDeferredForeignKeys(extractRequest.getLoadId(), targetNode); | ||
| if (isExtractRequestComplete(extractRequest, currentBatch, mode)) { | ||
| sqlTemplate.update(getSql("updateExtractRequestStatus"), ExtractStatus.OK.name(), new Date(), | ||
| currentBatch.getExtractRowCount(), currentBatch.getExtractMillis(), extractRequest.getRequestId()); | ||
| checkSendDeferredForeignKeys(extractRequest.getLoadId(), targetNode); | ||
| } else { | ||
| /* | ||
| * One batch finishing does not mean the request did. Marking the whole request OK from a single batch's counters is how a request | ||
| * ends up at OK with extracted_rows=0 while its batches are still RQ: every extraction selector filters on NE so nothing | ||
| * re-extracts it, extractBatch refuses to deliver an RQ batch, and nothing reconciles the two or raises an error. Accumulate the | ||
| * statistics instead, so progress stays visible without claiming completion. | ||
| */ | ||
| sqlTemplate.update(getSql("updateExtractRequestExtractedStats"), currentBatch.getExtractRowCount(), | ||
| currentBatch.getExtractMillis(), new Date(), extractRequest.getRequestId()); | ||
| log.debug("Batch {} finished but extract request {} for table {} is not complete; leaving its status at {}", | ||
| currentBatch.getBatchId(), extractRequest.getRequestId(), extractRequest.getTableName(), extractRequest.getStatus()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1720,13 +1733,19 @@ | |
| return transformExtractWriter; | ||
| } | ||
|
|
||
| /** When recovery last scanned for stuck extract requests, so it is not re-run faster than a request can become stale. */ | ||
| protected long lastStuckExtractRequestCheckMs; | ||
|
|
||
| @Override | ||
| public RemoteNodeStatuses queueWork(boolean force) { | ||
| final RemoteNodeStatuses statuses = new RemoteNodeStatuses(configurationService.getChannels(false)); | ||
| Node identity = nodeService.findIdentity(); | ||
| if (identity != null) { | ||
| if (force || clusterService.lock(ClusterConstants.INITIAL_LOAD_EXTRACT)) { | ||
| try { | ||
| // Before assigning threads, put back any request that is marked complete but demonstrably is not. | ||
| // The NE requests this produces are picked up by getExtractRequestNodes() in this same invocation. | ||
| recoverStuckExtractRequests(false); | ||
| updateExtractRequestsForThreading(); | ||
| List<NodeQueuePair> nodes = getExtractRequestNodes(); | ||
| for (NodeQueuePair pair : nodes) { | ||
|
|
@@ -2087,6 +2106,106 @@ | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Whether finishing this batch means the whole extract request is done. | ||
| * <p> | ||
| * The batch-status half of the test is exact rather than heuristic: {@code MultiBatchStagingWriter.close()} advances every remaining batch in the range, so | ||
| * after a completed extract-job run none of them are left at {@code RQ}. A request at {@code OK} with any batch in its range still {@code RQ} is therefore | ||
| * an impossible state rather than a healthy in-flight one, which is what makes both this guard and {@link #recoverStuckExtractRequests(boolean)} safe from | ||
| * false positives. The zero-row check is a heuristic and is called out as such where it appears. | ||
| */ | ||
| protected boolean isExtractRequestComplete(ExtractRequest request, OutgoingBatch currentBatch, ExtractMode mode) { | ||
| if (mode == ExtractMode.EXTRACT_ONLY) { | ||
| // changeBatchStatus does not persist in this mode, so the batch's own row would still read RQ. | ||
| return false; | ||
| } | ||
| if (request.getEndBatchId() > request.getStartBatchId() && currentBatch.getExtractRowCount() == 0 && request.getRows() > 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately this condition is hard to read... Making it hard to support.
I wonder if a more productive way to move forward is a demo with this issue discussed in a medium-sized gathering?.. Also, voluminous comments are a "code smell". It alerts reviewer that logic is so complex/convoluted/unrefined that code alone is not no longer self-documenting... |
||
| /* | ||
| * Heuristic, and checked before the query because it costs nothing: a multi-batch request whose expected row count is non-zero cannot be finished | ||
| * by a batch that extracted nothing, because those rows belong to another batch in the range. A legitimately empty final batch on a request that | ||
| * expects rows is the case this misjudges, and it errs towards leaving the request open, which the reconciler then resolves. | ||
| */ | ||
| return false; | ||
| } | ||
| return !hasRequestedBatchesInRange(request); | ||
| } | ||
|
|
||
| /** | ||
| * Whether any batch in the request's range is still requested. Deliberately not a {@code count(*)}: the answer is only ever used as a boolean, and a count | ||
| * has to visit every batch in the range, so across a load split into thousands of batches the cost would grow with the square of the batch count. Fetching | ||
| * one row stops at the first match, which in the common in-progress case is immediate. | ||
| */ | ||
| protected boolean hasRequestedBatchesInRange(ExtractRequest request) { | ||
| return !sqlTemplate.query(getSql("selectRequestedBatchesForExtractRequestSql"), 1, new LongMapper(), | ||
| new Object[] { request.getNodeId(), request.getStartBatchId(), request.getEndBatchId() }, null).isEmpty(); | ||
| } | ||
|
|
||
| /** Whether any batch in the request's range was already delivered, which makes an automatic restart unsafe. */ | ||
| protected boolean hasDeliveredBatchesInRange(ExtractRequest request) { | ||
| return !sqlTemplate.query(getSql("selectDeliveredBatchesForExtractRequestSql"), 1, new LongMapper(), | ||
| new Object[] { request.getNodeId(), request.getStartBatchId(), request.getEndBatchId() }, null).isEmpty(); | ||
| } | ||
|
|
||
| /** | ||
| * Return extract requests to {@code NE} when they are marked complete but demonstrably are not, so a load interrupted mid-extract resumes instead of | ||
| * sitting silently forever. Runs from {@link #queueWork(boolean)}, which also covers startup; a startup-only check would not have helped the reporting | ||
| * site, whose node ran three days in this state. | ||
| * <p> | ||
| * Requests whose range contains already-delivered batches are <em>not</em> restarted automatically. {@code restartExtractRequest} flips the whole range | ||
| * back to {@code RQ} through a statement with no status predicate, so it would re-send rows already committed at the target, and the target's own record of | ||
| * them can be missing (that is the data-integrity half of this defect). Those are reported and require {@code force}. | ||
| * | ||
| * @return the number of requests restarted | ||
| */ | ||
| @Override | ||
| public int recoverStuckExtractRequests(boolean force) { | ||
| if (!parameterService.is(ParameterConstants.INITIAL_LOAD_EXTRACT_REQUEST_RECOVERY_ENABLED, true)) { | ||
| return 0; | ||
| } | ||
| long thresholdMs = parameterService.getLong(ParameterConstants.INITIAL_LOAD_EXTRACT_REQUEST_RECOVERY_THRESHOLD_MS, 300000); | ||
| long now = System.currentTimeMillis(); | ||
| /* | ||
| * A request cannot qualify until it has been untouched for the threshold, so scanning faster than that only re-runs a query that cannot have a new | ||
| * answer. queueWork runs every 10 seconds; at the default threshold that would be 29 wasted scans out of every 30, each walking every completed request | ||
| * for this node plus a correlated probe into the outgoing batch table. | ||
| */ | ||
| if (!force && now - lastStuckExtractRequestCheckMs < thresholdMs) { | ||
| return 0; | ||
| } | ||
| lastStuckExtractRequestCheckMs = now; | ||
| List<ExtractRequest> stuck = sqlTemplateDirty.query(getSql("selectStuckExtractRequestsSql"), new ExtractRequestMapper(), | ||
| engine.getNodeId(), ExtractStatus.OK.name(), new Date(now - thresholdMs)); | ||
| int restarted = 0; | ||
| for (ExtractRequest request : stuck) { | ||
| if (hasDeliveredBatchesInRange(request) && !force) { | ||
| log.error("{} Some of those batches were already delivered, so restarting it would re-send rows that are already committed at the target. " | ||
| + "This load will not progress on its own: either truncate the target table and force recovery, or cancel the load.", | ||
| describeStuckRequest(request)); | ||
|
Check warning on line 2183 in symmetric-core/src/main/java/org/jumpmind/symmetric/service/impl/DataExtractorService.java
|
||
| continue; | ||
| } | ||
| log.warn("{} That cannot happen on a completed extract, so it is being re-queued for extraction.", describeStuckRequest(request)); | ||
|
Check warning on line 2186 in symmetric-core/src/main/java/org/jumpmind/symmetric/service/impl/DataExtractorService.java
|
||
| restartExtractRequest(request); | ||
| restarted++; | ||
| } | ||
| if (restarted > 0) { | ||
| log.warn("Recovered {} stuck extract request(s)", restarted); | ||
| } | ||
| return restarted; | ||
| } | ||
|
|
||
| protected String describeStuckRequest(ExtractRequest request) { | ||
| return String.format("Extract request %d for table %s (load %d, node %s) is marked %s with %d of %d rows extracted, while batches %d through %d are " | ||
| + "still requested.", request.getRequestId(), request.getTableName(), request.getLoadId(), request.getNodeId(), ExtractStatus.OK.name(), | ||
| request.getExtractedRows(), request.getRows(), request.getStartBatchId(), request.getEndBatchId()); | ||
| } | ||
|
|
||
| /** Load the batch range and any child requests, then hand off to {@link #restartExtractRequest(List, ExtractRequest, List)}. */ | ||
| protected void restartExtractRequest(ExtractRequest request) { | ||
| List<OutgoingBatch> batches = outgoingBatchService.getOutgoingBatchRange(request.getStartBatchId(), request.getEndBatchId()).getBatches(); | ||
| List<ExtractRequest> childRequests = request.getParentRequestId() == 0 ? getExtractChildRequestsForNode(request) : null; | ||
| restartExtractRequest(batches, request, childRequests); | ||
| } | ||
|
|
||
| protected void restartExtractRequest(List<OutgoingBatch> batches, ExtractRequest request, List<ExtractRequest> childRequests) { | ||
| /* | ||
| * This extract request was interrupted and must start over | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is new parameter is needed?
I was hoping the issue is fixed without user needing to analyse it and set parameters?