-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix(zcncore): added MakeSCRestAPICall and fixed fast confirmation on GetInfo #718
Conversation
…get info from sharders
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.
lgtm
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.
Please fix the macos build error
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.
For the FromConsensus, we actually don't have to do another process logic, we can do it in simliar way as the FromAll, but only send requests to part of the sharders, and collect the failed requets number as I suggested before.
the request with be cancelled by WithTimeout if the sharder is offline or slow. and next sharder will be pushed to |
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 first version of this PR is actually 90 percent ready, we only need to limit the request size in MakeSCRestAPICall and check the failed count, let's not overcomplicate it.
zcncore/transaction_query_base.go
Outdated
path := fmt.Sprintf("/v1/screst/%v/%v", scAddress, relativePath) | ||
query := withParams(path, Params(params)) | ||
|
||
tq, err := NewTransactionQuery(util.Shuffle(_config.chain.Sharders)) |
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 main changes we need is in this MakeSCRestAPICall
function, that we limit the shaders number instead of passing all sharders to NewTransactionQuery
. Then call the FromAll() to send requests to those selected sharders. We don't need the fromConsensus
.
reqSharders := util.Shuffle(copyOf(_config.chain.Sharders))[:reqNum] // util.Shuffle(_config.chain.Sharders) // this shuffle will change the Sharders slice, it's not thread safe.
tq, err := NewTransactionQuery(reqSharders)
...
Manual system tests [cancelled] with the following config
|
Manual system tests [failure] with the following config
|
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.
LGTM.
@cnlangzi can you create an issue for the refactoring I mentioned below, thanks.
@peterlimg please also check https://0chain.slack.com/archives/D02FLSUG42C/p1674110680380779?thread_ts=1673955258.028839&cid=D02FLSUG42C. I left a question about consensus checking
|
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.
LGTM
Changes
Fixes
Tests
Tasks to complete before merging PR:
Associated PRs (Link as appropriate):