-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Feature] Support Cluster Snapshot Backup: SQL Interface and meta data (part 1) #54447
Conversation
public void write(DataOutput out) throws IOException { | ||
Text.writeString(out, GsonUtils.GSON.toJson(this)); | ||
} | ||
} |
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.
The most risky bug in this code is:
Typo in the method call createAutomatedSnaphot
, which should be createAutomatedSnapshot
.
You can modify the code like this:
private void createSnapshotIfJobIsFinished() {
GlobalStateMgr.getCurrentState().getClusterSnapshotMgr().createAutomatedSnapshot(this);
}
@Override | ||
public void gsonPostProcess() throws IOException { | ||
} | ||
} |
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.
The most risky bug in this code is:
In the load
method, the deserialized object data
is not used to update the state of the current ClusterSnapshotMgr
. This means that the data read from the SRMetaBlockReader
, which presumably represents a saved state, does nothing to restore or set the internal state of the object using it.
You can modify the code like this:
public void load(SRMetaBlockReader reader)
throws SRMetaBlockEOFException, IOException, SRMetaBlockException {
ClusterSnapshotMgr data = reader.readJson(ClusterSnapshotMgr.class);
this.automatedSnapshotSvName = data.automatedSnapshotSvName;
this.automatedSnapshot = data.automatedSnapshot;
this.historyAutomatedSnapshotJobs = data.historyAutomatedSnapshotJobs;
this.remoteStorage = data.remoteStorage;
this.locationWithServiceId = data.locationWithServiceId;
}
String json = GsonUtils.GSON.toJson(this); | ||
Text.writeString(out, json); | ||
} | ||
} |
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.
The most risky bug in this code is:
The read
method may fail if the input JSON does not match the expected structure or if required fields are missing or null.
You can modify the code like this:
public static ClusterSnapshotLog read(DataInput in) throws IOException {
try {
String json = Text.readString(in);
ClusterSnapshotLog log = GsonUtils.GSON.fromJson(json, ClusterSnapshotLog.class);
// Add checks to ensure required fields are present and valid
if (log.type == null) {
throw new IOException("ClusterSnapshotLog type cannot be null");
}
return log;
} catch (Exception e) {
throw new IOException("Failed to read ClusterSnapshotLog from input", e);
}
}
adminSetAutomatedSnapshotStatement | ||
: ADMIN SET AUTOMATED CLUSTER SNAPSHOT (ON (STORAGE VOLUME svName=identifier)? | OFF) | ||
; |
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.
adminSetAutomatedSnapshotStatement | |
: ADMIN SET AUTOMATED CLUSTER SNAPSHOT (ON (STORAGE VOLUME svName=identifier)? | OFF) | |
; | |
adminSetAutomatedSnapshotOnStatement | |
: ADMIN SET AUTOMATED CLUSTER SNAPSHOT ON (STORAGE VOLUME svName=identifier)? | |
; | |
adminSetAutomatedSnapshotOffStatement | |
: ADMIN SET AUTOMATED CLUSTER SNAPSHOT OFF | |
; |
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.
Updated
|
||
public ClusterSnapshotMgr() {} | ||
|
||
public void addAutomatedSnapshotRequest(String storageVolumeName, boolean isReplay) { |
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.
public void addAutomatedSnapshotRequest(String storageVolumeName, boolean isReplay) { | |
// Turn on automated snapshot, use stmt for extension in future | |
public void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotOnStmt stmt) { |
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.
Updated
return getAutomatedSnapshot() != null; | ||
} | ||
|
||
public boolean containAutomatedSnapshotRequest() { |
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.
public boolean containAutomatedSnapshotRequest() { | |
public boolean isAutomatedSnapshotOn() { |
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.
Updated
return snapshot.getSnapshotName(); | ||
} | ||
|
||
public void dropAutomatedSnapshotRequest(boolean isReplay) { |
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.
public void dropAutomatedSnapshotRequest(boolean isReplay) { | |
// Turn off automated snapshot, use stmt for extension in future | |
public void setAutomatedSnapshotOff(AdminSetAutomatedSnapshotOffStmt stmt) { |
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.
Updated
@Override | ||
public void write(DataOutput out) throws IOException { | ||
Text.writeString(out, GsonUtils.GSON.toJson(this)); | ||
} |
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 needed
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.
Updated
@SerializedName(value = "automatedSnapshot") | ||
private ClusterSnapshot automatedSnapshot = null; | ||
@SerializedName(value = "historyAutomatedSnapshotJobs") | ||
private List<ClusterSnapshotJob> historyAutomatedSnapshotJobs = Lists.newArrayList(); |
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 thread-safe ?
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.
Updated
38fa362
to
0f400f3
Compare
public void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotStmt stmt) { | ||
setAutomatedSnapshotOn(stmt, false); | ||
} | ||
|
||
protected void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotStmt stmt, boolean isReplay) { | ||
String storageVolumeName = stmt.getStorageVolumeName(); | ||
if (!isReplay) { | ||
ClusterSnapshotLog log = new ClusterSnapshotLog(); | ||
log.setCreateSnapshotNamePrefix(AUTOMATED_NAME_PREFIX, storageVolumeName); | ||
GlobalStateMgr.getCurrentState().getEditLog().logClusterSnapshotLog(log); | ||
} | ||
automatedSnapshotSvName = storageVolumeName; | ||
} |
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.
public void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotStmt stmt) { | |
setAutomatedSnapshotOn(stmt, false); | |
} | |
protected void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotStmt stmt, boolean isReplay) { | |
String storageVolumeName = stmt.getStorageVolumeName(); | |
if (!isReplay) { | |
ClusterSnapshotLog log = new ClusterSnapshotLog(); | |
log.setCreateSnapshotNamePrefix(AUTOMATED_NAME_PREFIX, storageVolumeName); | |
GlobalStateMgr.getCurrentState().getEditLog().logClusterSnapshotLog(log); | |
} | |
automatedSnapshotSvName = storageVolumeName; | |
} | |
public void setAutomatedSnapshotOn(AdminSetAutomatedSnapshotStmt stmt) { | |
setAutomatedSnapshotOn(stmt.getStorageVolumeName()); | |
ClusterSnapshotLog log = new ClusterSnapshotLog(); | |
log.setCreateSnapshotNamePrefix(AUTOMATED_NAME_PREFIX, storageVolumeName); | |
GlobalStateMgr.getCurrentState().getEditLog().logClusterSnapshotLog(log); | |
} | |
protected void setAutomatedSnapshotOn(String storageVolumeName) { | |
automatedSnapshotSvName = storageVolumeName; | |
} |
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.
Updated
@@ -97,6 +97,8 @@ public int getId() { | |||
|
|||
public static final SRMetaBlockID PIPE_MGR = new SRMetaBlockID(32); | |||
|
|||
public static final SRMetaBlockID CLOULD_NATIVE_SNAPSHOT_MGR = new SRMetaBlockID(33); |
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.
CLUSTER_SNAPSHOT_MGR
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.
Updated
|
||
import com.starrocks.sql.parser.NodePosition; | ||
|
||
public class AdminSetAutomatedSnapshotStmt extends DdlStmt { |
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.
public class AdminSetAutomatedSnapshotStmt extends DdlStmt { | |
public class AdminSetAutomatedSnapshotOnStmt extends DdlStmt { |
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.
Updated
|
||
import com.starrocks.sql.parser.NodePosition; | ||
|
||
public class AdminSetOffAutomatedSnapshotStmt extends DdlStmt { |
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.
public class AdminSetOffAutomatedSnapshotStmt extends DdlStmt { | |
public class AdminSetAutomatedSnapshotOffStmt extends DdlStmt { |
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.
Updated
@@ -734,6 +736,14 @@ syncStatement | |||
: SYNC | |||
; | |||
|
|||
adminSetAutomatedSnapshotStatement |
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.
adminSetAutomatedSnapshotStatement | |
adminSetAutomatedSnapshotOnStatement |
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.
Updated
: ADMIN SET AUTOMATED CLUSTER SNAPSHOT ON (STORAGE VOLUME svName=identifier)? | ||
; | ||
|
||
adminSetOffAutomatedSnapshotStatement |
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.
adminSetOffAutomatedSnapshotStatement | |
adminSetAutomatedSnapshotOffStatement |
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.
Updated
public void setAutomatedSnapshotOff(AdminSetOffAutomatedSnapshotStmt stmt) { | ||
setAutomatedSnapshotOff(stmt, false); | ||
} | ||
|
||
protected void setAutomatedSnapshotOff(AdminSetOffAutomatedSnapshotStmt stmt, boolean isReplay) { | ||
if (!isReplay) { | ||
ClusterSnapshotLog log = new ClusterSnapshotLog(); | ||
log.setDropSnapshot(AUTOMATED_NAME_PREFIX); | ||
GlobalStateMgr.getCurrentState().getEditLog().logClusterSnapshotLog(log); | ||
} | ||
|
||
// drop AUTOMATED snapshot | ||
automatedSnapshotSvName = ""; | ||
} |
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.
same as above
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.
Updated
…a (part 1) Signed-off-by: srlch <[email protected]>
0891fe8
to
3bea59e
Compare
Signed-off-by: srlch <[email protected]>
3bea59e
to
46d5c78
Compare
Quality Gate passedIssues Measures |
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 79 / 115 (68.70%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
https://github.com/Mergifyio backport branch-3.4 |
✅ Backports have been created
|
…a (part 1) (#54447) Signed-off-by: srlch <[email protected]> (cherry picked from commit be70af0) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/qe/DDLStmtExecutor.java
…a (part 1) (backport #54447) (#54505) Signed-off-by: srlch <[email protected]>
…a (part 1) (StarRocks#54447) Signed-off-by: srlch <[email protected]>
What I'm doing:
This is part 1 for support backup cluster snapshot:
Introduce SQL Interface for TURN ON and TRUN OFF the automated cluster snapshot task:
Fixes #53867
#53867
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: