-
Notifications
You must be signed in to change notification settings - Fork 466
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
Cleanup: Remove indexer v1 from codebase #5477
Conversation
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.
Looks pretty sensible to me, but I don't think we can remove the config value.
@@ -237,10 +237,6 @@ type Local struct { | |||
// the max size the sync server would return | |||
TxSyncServeResponseSize int `version[3]:"1000000"` | |||
|
|||
// IsIndexerActive indicates whether to activate the indexer for fast retrieval of transactions | |||
// Note -- Indexer cannot operate on non Archival nodes | |||
IsIndexerActive bool `version[3]:"false"` |
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.
Need to leave this in or else we'd config file parsing failures. Even though most configs will have it set to false already, I'm pretty sure unknown fields are illegal.
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.
I thought that too, but check out the config test - it's loading config versions prior to v28 that have the property without 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.
Woah. I wonder if its always been that way. Thanks for the correction!
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.
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.
// To unmarshal JSON into a struct, Unmarshal matches incoming object
// keys to the keys used by Marshal (either the struct field name or its tag),
// preferring an exact match but also accepting a case-insensitive match. By
// default, object keys which don't have a corresponding struct field are
// ignored (see Decoder.DisallowUnknownFields for an alternative).
indeed, what a surprize, I thought it fails like msgp
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.
I checked IsIndexerActive=true also deserialized without problems and matches the documentation.
Not sure tho if silently removal config values is a good practice? Maybe there should be some kind of deprecated fields and warning in node.log?
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.
It would be cool if our config tagging system supported something like
IsIndexerActive bool `version[3]:"false" version[28]:DEPRECATED`
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.
so you could get deprecation warnings logged at startup time
Co-authored-by: Will Winder <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #5477 +/- ##
==========================================
- Coverage 55.64% 52.74% -2.91%
==========================================
Files 448 446 -2
Lines 63418 63197 -221
==========================================
- Hits 35291 33333 -1958
- Misses 25743 27401 +1658
- Partials 2384 2463 +79
... and 117 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
I guess removal is fine, the config value removal is questionable - yes, it works but is it a good practice? Opinions?
@@ -237,10 +237,6 @@ type Local struct { | |||
// the max size the sync server would return | |||
TxSyncServeResponseSize int `version[3]:"1000000"` | |||
|
|||
// IsIndexerActive indicates whether to activate the indexer for fast retrieval of transactions | |||
// Note -- Indexer cannot operate on non Archival nodes | |||
IsIndexerActive bool `version[3]:"false"` |
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.
I checked IsIndexerActive=true also deserialized without problems and matches the documentation.
Not sure tho if silently removal config values is a good practice? Maybe there should be some kind of deprecated fields and warning in node.log?
I'm having trouble of thinking of the downsides of removing it - we know serialization/deserialization works, I'm thinking a call out in the release notes + potentially mentioning in the next consensus upgrade notes might help? |
I checked the failure
and it comes from the fact There is still an open question what to do with |
Summary
Removes the indexer v1 from go-algorand. The API it supported was removed in 2022, making this code no longer needed. Deprecates the command line flag to enable the indexer and removes the configuration file flag entirely.
Test Plan
Unit tests (including backwards compatibiltiy of flag being present in prior config versions but not this one), functional/e2e tests.