github-actions[bot] commented on code in PR #66154:
URL: https://github.com/apache/doris/pull/66154#discussion_r3665969062
##########
fe/fe-core/src/main/java/org/apache/doris/master/Checkpoint.java:
##########
@@ -180,7 +183,9 @@ public synchronized void doCheckpoint() throws
CheckpointException {
// If failed, just return
env = Env.getCurrentEnv();
createStaticFieldForCkpt();
- env.loadImage(imageDir);
+ env.loadImage(checkpointImageFile, replayedJournalId);
+ env.publishCheckpointImage(checkpointImageFile);
Review Comment:
Same-version publication can race an active `/image` response.
`writeFileResponse()` records `File.length()` and hashes one `FileInputStream`,
then `getFile()` opens the pathname again for the body. If this replacement
lands between those opens, the headers describe the old inode while the body
comes from the new one. `MetaHelper.getRemoteFile()` rejects the size/MD5
mismatch, and `Env.getNewImage()` propagates that `IOException` during FE
bootstrap. Please make the response metadata and body come from one stable open
handle/snapshot, or coordinate both leader and follower same-version
replacement with image serving.
##########
fe/fe-core/src/main/java/org/apache/doris/master/Checkpoint.java:
##########
@@ -126,6 +127,7 @@ public synchronized void doCheckpoint() throws
CheckpointException {
// No new finalized journals beyond the latest image.
// But in cloud mode, we may still want to force a checkpoint
if the latest image file is expired.
// This helps that image can keep the newer table version,
partition version, tablet stats.
+ checkPointVersion = imageVersion;
LOG.info("Trigger checkpoint in cloud mode because latest
image is expired. "
Review Comment:
Because this refresh deliberately keeps version `N`, a peer that misses the
one-time `/put?version=N` cannot catch up later. On restart, `/info` exposes
only `imageSeq`, and `getNewImage()` downloads only when the remote sequence is
strictly greater; old-content `image.N` and refreshed-content `image.N`
therefore compare equal. The peer keeps the stale on-disk tablet-stat snapshot
until runtime collection rebuilds it. Please add a refresh
generation/mtime/checksum to synchronization, retry failed equal-version
pushes, or otherwise make restart replace an older-content image with the same
journal version.
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/EnvTest.java:
##########
@@ -183,4 +191,46 @@ public void
testSetLdapDefaultRolesConfigRefreshesLdapCache() throws Exception {
LdapConfig.ldap_default_roles = oldLdapDefaultRoles;
}
}
+
+ @Test
Review Comment:
These tests exercise only the raw move. They stay green if the reported fix
at `Checkpoint.java:130` is removed, or if checkpoint validation is
skipped/reordered, because neither case calls `Checkpoint.doCheckpoint()`,
`saveImageToCheckpoint()`, or `loadImage(File,long)`. Please add
checkpoint-level coverage with a stale `image.<N>` and finalized journal below
`N`, plus a corrupt generated `image.ckpt`, to prove regeneration targets `N`
and validation failure leaves the prior numbered image intact.
--
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]
