Skip to content
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

V1 Deprecation: Remove algod and kmd client affinity and consolidate APIs to use newest version #4641

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Oct 13, 2022

Summary

As part of sunsetting V1 algod APIs, we want goal to only use V2 algod APIs. This PR removes client affinity and consolidates all algod API calls to use the V2 version (mostly in test calls). kmd affinity is also removed, but this does not affect any functionality since kmd only has one version.

This does not change any goal behavior, as it fetched the V2 algod client by default: https://github.com/algorand/go-algorand/pull/4641/files#diff-75c865ad4f92ef3366534a2489ccd8aaf03e31a1b17027d64fe2216076d3c067L384

Test Plan

CI Tests.

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #4641 (c7f6cea) into master (bb7c59f) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4641      +/-   ##
==========================================
+ Coverage   54.34%   54.39%   +0.04%     
==========================================
  Files         402      402              
  Lines       51793    51786       -7     
==========================================
+ Hits        28149    28168      +19     
+ Misses      21276    21253      -23     
+ Partials     2368     2365       -3     
Impacted Files Coverage Δ
cmd/goal/commands.go 10.44% <ø> (+0.03%) ⬆️
libgoal/libgoal.go 2.70% <ø> (+0.02%) ⬆️
data/transactions/verify/txn.go 76.19% <0.00%> (-0.96%) ⬇️
network/wsNetwork.go 64.76% <0.00%> (+0.19%) ⬆️
catchup/service.go 69.62% <0.00%> (+0.74%) ⬆️
agreement/proposalManager.go 98.03% <0.00%> (+1.96%) ⬆️
agreement/cryptoVerifier.go 69.71% <0.00%> (+2.11%) ⬆️
network/wsPeer.go 67.64% <0.00%> (+2.13%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

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

@algochoi algochoi marked this pull request as ready for review October 13, 2022 20:34
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would approve except for the question of users of MakeRestClient - we need to make sure, for example, that we aren't making the loadgenerator slower.

Comment on lines 89 to +92
func MakeRestClient(url url.URL, apiToken string) RestClient {
return RestClient{
serverURL: url,
apiToken: apiToken,
versionAffinity: APIVersionV1,
serverURL: url,
apiToken: apiToken,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we understand why this was using APIVersionV1? It means, for example, that the loadgenerator cmd/loadgenerator/main.go uses V1 today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure.

  • Scanning git history, I see 1750fb5#diff-c8e8e0d825d7542580daf188e1c5f6b4f079c888d10dfb2544a8a75b6419d0b4R72. When version affinity was introduced, MakeRestClient was locked to v1.
  • My unsubstantiated guess is: Using v1 simplified other ongoing work at the time and switching to v2 got lost in the shuffle.
  • Additionally - For load generator algod API usage, I scanned endpoint implementations in v1 + v2 and spotted no meaningful differences. Of course, it'd be preferable for someone more familiar with load generator to vouch and/or run a test.

@algorandskiy
Copy link
Contributor

@cce @brianolson could you look into it to double check the loadgenerator does not break?

@brianolson
Copy link
Contributor

pingpong has several V1 dependencies

@brianolson
Copy link
Contributor

I didn't do a performance stress test but loadgenerator does basically work with this change in.

loadgenerator -d node -config '{"RoundModulator":10,"Fee":10000,"TxnsToSend":10}'

@algochoi
Copy link
Contributor Author

@brianolson Thank you for checking - visually, it doesn't look like the v2 API will cause any harm. Are reviewers OK with moving forward with this change then?

@michaeldiamant
Copy link
Contributor

Barring objections, I'll merge the PR near EOD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update go-algorand internals to use Algod V2 API (No more V1 dependencies)
5 participants