Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

api: StartFastCatchup initialize parameter #5752

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

winder
Copy link
Contributor

@winder winder commented Sep 25, 2023

Summary

Adds a new initialize query parameter to the StartCatchup admin API endpoint. It introduces a new concept for when a node is considered initialized compared to a catchpoint, which is defined by the value of the initialize parameter. The catchup request will be a no-op unless the catchpoint advances the node by at least initialize rounds.

Test Plan

New unit test.

@winder winder requested review from cce and jannotti September 25, 2023 14:11
@winder winder assigned winder and unassigned cce and jannotti Sep 25, 2023
@jannotti
Copy link
Contributor

Your reasoning for this made sense for adding this at the time, but now I can't recall it. Why would the catchpoint endpoint be called if the node is already close to catchup? I thought I remembered something like, "Once you're in catchup mode, you would keep trying to fast catchup." But that doesn't fit with what this does.

Is the problem that there are automated situations in which the catchpoint endpoint is called? Maybe in sandbox?

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #5752 (6f07de0) into master (5570e88) will decrease coverage by 0.45%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5752      +/-   ##
==========================================
- Coverage   55.50%   55.06%   -0.45%     
==========================================
  Files         473      473              
  Lines       66675    66683       +8     
==========================================
- Hits        37007    36718     -289     
- Misses      27157    27437     +280     
- Partials     2511     2528      +17     
Files Changed Coverage Δ
daemon/algod/api/server/v2/handlers.go 0.79% <0.00%> (-0.01%) ⬇️

... and 42 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@winder
Copy link
Contributor Author

winder commented Sep 25, 2023

Your reasoning for this made sense for adding this at the time, but now I can't recall it. Why would the catchpoint endpoint be called if the node is already close to catchup? I thought I remembered something like, "Once you're in catchup mode, you would keep trying to fast catchup." But that doesn't fit with what this does.

@jannotti It is mainly to simplify scripts/tools. i.e. when Conduit starts up, it has some logic to check the latest catchpoint and the latest round and it has to make a decision about whether or not to run fast-catchup. The docker container has a similar feature, but is a bash script. Since I'm now implementing this logic a 2nd time I figured it makes sense to put it in a common place.

With this feature, tools can run fast catchup unconditionally and things will just work. So it's harder to make a mistake, and they don't have to figure out our catchpoint label encoding.

@winder winder marked this pull request as ready for review September 25, 2023 14:45
@jannotti
Copy link
Contributor

Your reasoning for this made sense for adding this at the time, but now I can't recall it. Why would the catchpoint endpoint be called if the node is already close to catchup? I thought I remembered something like, "Once you're in catchup mode, you would keep trying to fast catchup." But that doesn't fit with what this does.

@jannotti It is mainly to simplify scripts/tools. i.e. when Conduit starts up, it has some logic to check the latest catchpoint and the latest round and it has to make a decision about whether or not to run fast-catchup. The docker container has a similar feature, but is a bash script. Since I'm now implementing this logic a 2nd time I figured it makes sense to put it in a common place.

With this feature, tools can run fast catchup unconditionally and things will just work. So it's harder to make a mistake, and they don't have to figure out our catchpoint label encoding.

I see, that makes sense. Is it worthwhile to make the boolean parameter an integer, instead of defining 1M as the unconditional distance?

@winder
Copy link
Contributor Author

winder commented Sep 25, 2023

Your reasoning for this made sense for adding this at the time, but now I can't recall it. Why would the catchpoint endpoint be called if the node is already close to catchup? I thought I remembered something like, "Once you're in catchup mode, you would keep trying to fast catchup." But that doesn't fit with what this does.

@jannotti It is mainly to simplify scripts/tools. i.e. when Conduit starts up, it has some logic to check the latest catchpoint and the latest round and it has to make a decision about whether or not to run fast-catchup. The docker container has a similar feature, but is a bash script. Since I'm now implementing this logic a 2nd time I figured it makes sense to put it in a common place.
With this feature, tools can run fast catchup unconditionally and things will just work. So it's harder to make a mistake, and they don't have to figure out our catchpoint label encoding.

I see, that makes sense. Is it worthwhile to make the boolean parameter an integer, instead of defining 1M as the unconditional distance?

I like that idea. I'll change this to an int.

@winder
Copy link
Contributor Author

winder commented Sep 25, 2023

@jannotti your suggestion has been implemented, please take another look.

@jannotti
Copy link
Contributor

Will autogen SDKs automatically pick up the new parameter?

@winder
Copy link
Contributor Author

winder commented Sep 25, 2023

Will autogen SDKs automatically pick up the new parameter?

No, the SDKs don't support admin endpoints like this one.

@winder winder merged commit 17871a3 into algorand:master Sep 25, 2023
@winder winder deleted the will/fast-catchup-init branch September 25, 2023 19:45
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants