-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[BugFix] follower sync dictionary refresh state #47802
Conversation
gensrc/thrift/FrontendService.thrift
Outdated
@@ -1890,5 +1901,7 @@ service FrontendService { | |||
|
|||
TListSessionsResponse listSessions(1: TListSessionsRequest request) | |||
TGetTemporaryTablesInfoResponse getTemporaryTablesInfo(1: TGetTemporaryTablesInfoRequest request) | |||
|
|||
TUpdateFollowerDictionaryStateResult updateFollowerDictionaryState(1: TUpdateFollowerDictionaryStateParams request) | |||
} | |||
|
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:
Missing semicolon after last_success_finished_time
.
You can modify the code like this:
struct TUpdateFollowerDictionaryStateParams {
1: optional i64 dictionary_id;
2: optional i64 success_txn_id;
3: optional bool error;
4: optional string error_msg;
5: optional i64 last_success_finished_time;
}
} catch (Exception e) { | ||
LOG.warn("sync dictionary refresh state failed, fe: {}", fe.getHost(), e); | ||
} | ||
} | ||
} | ||
|
||
@Override |
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:
Failure to synchronize dictionary states can lead to inconsistencies among followers.
You can modify the code like this:
public TUpdateFollowerDictionaryStateResult updateFollowerDictionaryState(TUpdateFollowerDictionaryStateParams request) {
TUpdateFollowerDictionaryStateResult result = new TUpdateFollowerDictionaryStateResult();
long leaderDictionaryId = request.getDictionary_id(); // Use getter methods for encapsulation
long leaderSuccessTxnId = request.getSuccess_txn_id(); // Same here
boolean error = request.isError(); // get request parameters properly
String errMsg = request.getError_msg(); // get request parameters properly
long lastSuccessFinishedTime = request.getLast_success_finished_time(); // use getter method
Dictionary dictionary = dictionariesMapById.get(leaderDictionaryId);
if (dictionary == null) {
LOG.warn("Dictionary does not exist when sync the refresh state to follower, dictionary id: {}", leaderDictionaryId);
return result;
}
if (!error) {
GlobalStateMgr.getCurrentState().getDictionaryMgr().updateLastSuccessTxnId(leaderDictionaryId, leaderSuccessTxnId);
dictionary.setFinished(lastSuccessFinishedTime); // update with correct finished time
dictionary.setErrorMsg(""); // reset error msg
} else if (dictionary.getIgnoreFailedRefresh()) {
dictionary.resetStateBeforeRefresh();
dictionary.setErrorMsg("Cancelled and rollback to previous state, errMsg: " + errMsg);
} else {
dictionary.setCancelled();
dictionary.setErrorMsg(errMsg);
}
return result;
}
Additional explanation:
- Getter Methods: Ensure you're using getter methods (
request.getDictionary_id()
, etc.) instead of accessing fields directly. This follows Java best practices for encapsulation. - Proper Error Handling: Consistently handle any potential errors by accurately updating the
Dictionary
object. - Consistency Check: Ensure consistency checks between leader and followers are adequately addressed, which includes proper synchronization mechanisms if needed.
a70d0a6
to
5392c2f
Compare
462ec7c
to
173b5e1
Compare
443058a
to
29453f1
Compare
fe/fe-core/src/main/java/com/starrocks/catalog/DictionaryMgr.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/com/starrocks/catalog/DictionaryMgr.java
Outdated
Show resolved
Hide resolved
62134de
to
3d9d702
Compare
Why I'm doing: In the current implementations, follower will not be synced any information about the dictionary refreshing which is a problem. What I'm doing: Reuse dictionaryMgrInfo and DICTIONARY_MODIFY log to save the dictionary state information. Signed-off-by: srlch <[email protected]>
Signed-off-by: srlch <[email protected]>
3d9d702
to
33738ca
Compare
Signed-off-by: srlch <[email protected]>
Quality Gate passedIssues Measures |
[FE Incremental Coverage Report]✅ pass : 64 / 74 (86.49%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
@Mergifyio backport branch-3.3 |
✅ Backports have been created
|
Signed-off-by: srlch <[email protected]> (cherry picked from commit 7cb4a4a)
…8252) Co-authored-by: srlch <[email protected]>
Why I'm doing:
In the current implementations, follower will not be synced any information about the dictionary refreshing which is a problem.
What I'm doing:
Reuse dictionaryMgrInfo and DICTIONARY_MODIFY log to save the dictionary state information.
Fixes #issue
https://github.com/StarRocks/StarRocksTest/issues/7961
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: