Cleanup: Remove indexer v1 from codebase #5477

gmalouf · 2023-06-16T20:40:52Z

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.

…ated configs.

winder

Looks pretty sensible to me, but I don't think we can remove the config value.

cmd/goal/node.go

winder · 2023-06-16T20:48:40Z

config/localTemplate.go

@@ -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"`


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.

I thought that too, but check out the config test - it's loading config versions prior to v28 that have the property without error.

Woah. I wonder if its always been that way. Thanks for the correction!

https://github.com/algorand/go-algorand/blob/master/config/config_test.go#L292

// 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

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?

It would be cool if our config tagging system supported something like

IsIndexerActive bool `version[3]:"false" version[28]:DEPRECATED`

so you could get deprecation warnings logged at startup time

Co-authored-by: Will Winder <wwinder.unh@gmail.com>

codecov · 2023-06-16T21:17:54Z

Codecov Report

Merging #5477 (02644a8) into master (07f9431) will decrease coverage by 2.91%.
The diff coverage is 57.14%.

@@            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

Impacted Files	Coverage Δ
config/localTemplate.go	`33.84% <ø> (-36.93%)`	⬇️
daemon/algod/api/server/v2/test/helpers.go	`70.83% <ø> (-3.91%)`	⬇️
node/node.go	`3.64% <0.00%> (-0.40%)`	⬇️
cmd/goal/node.go	`12.46% <80.00%> (+0.67%)`	⬆️

... and 117 files with indirect coverage changes

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

bbroder-algo

lgtm

algorandskiy

I guess removal is fine, the config value removal is questionable - yes, it works but is it a good practice? Opinions?

algorandskiy · 2023-06-20T20:51:34Z

config/localTemplate.go

@@ -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"`


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?

gmalouf · 2023-06-20T22:45:39Z

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?

algorandskiy · 2023-06-21T17:36:29Z

I checked the failure

=== FAIL: node/indexer TestExampleTestSuite (0.29s)
    indexer_test.go:88: 
        	Error:      	Received unexpected error:
        	            	UNIQUE constraint failed: transactions.txid
        	Test:       	TestExampleTestSuite

and it comes from the fact generateTestObjects generates duplicate time to time b/c go1.20 changed random seeding looks like.

There is still an open question what to do with IsIndexerActive removal.

gmalouf added 2 commits June 16, 2023 15:52

Remove indexer v1 from go-algorand codebase.

ff87e54

Removes isIndexerActive config property from local template and gener…

5f766d0

…ated configs.

gmalouf added the tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead label Jun 16, 2023

gmalouf requested review from algorandskiy and winder June 16, 2023 20:41

gmalouf changed the title ~~Cleanup: Removes indexer v1 from codebase~~ Cleanup: Remove indexer v1 from codebase Jun 16, 2023

gmalouf added Enhancement and removed tech debt Things that need re-work for simplification / sanitization to reduce implementation overhead labels Jun 16, 2023

winder suggested changes Jun 16, 2023

View reviewed changes

Update cmd/goal/node.go

02644a8

Co-authored-by: Will Winder <wwinder.unh@gmail.com>

gmalouf requested review from bbroder-algo and winder June 20, 2023 17:23

gmalouf marked this pull request as ready for review June 20, 2023 17:26

bbroder-algo approved these changes Jun 20, 2023

View reviewed changes

winder approved these changes Jun 20, 2023

View reviewed changes

algorandskiy reviewed Jun 20, 2023

View reviewed changes

algorandskiy merged commit 5f496cf into algorand:master Jun 21, 2023

algorandskiy added Skip-Release-Notes Enhancement and removed Enhancement Skip-Release-Notes labels Jun 21, 2023

Algo-devops-service mentioned this pull request Jul 11, 2023

go-algorand 3.17.0-beta Release PR #5541

Merged

onetechnical mentioned this pull request Jul 24, 2023

go-algorand 3.17.0-beta Release PR #5601

Merged

Algo-devops-service mentioned this pull request Aug 3, 2023

go-algorand 3.17.0-stable Release PR #5633

Merged

gmalouf deleted the remove-indexer-v1 branch August 18, 2023 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: Remove indexer v1 from codebase #5477

Cleanup: Remove indexer v1 from codebase #5477

gmalouf commented Jun 16, 2023

winder left a comment

winder Jun 16, 2023 •

edited

Loading

gmalouf Jun 16, 2023 •

edited

Loading

winder Jun 16, 2023

gmalouf Jun 16, 2023

algorandskiy Jun 20, 2023

algorandskiy Jun 20, 2023

cce Jun 26, 2023

cce Jun 26, 2023

codecov bot commented Jun 16, 2023 •

edited

Loading

bbroder-algo left a comment

algorandskiy left a comment

algorandskiy Jun 20, 2023

gmalouf commented Jun 20, 2023

algorandskiy commented Jun 21, 2023

Cleanup: Remove indexer v1 from codebase #5477

Cleanup: Remove indexer v1 from codebase #5477

Conversation

gmalouf commented Jun 16, 2023

Summary

Test Plan

winder left a comment

Choose a reason for hiding this comment

winder Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

gmalouf Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

winder Jun 16, 2023

Choose a reason for hiding this comment

gmalouf Jun 16, 2023

Choose a reason for hiding this comment

algorandskiy Jun 20, 2023

Choose a reason for hiding this comment

algorandskiy Jun 20, 2023

Choose a reason for hiding this comment

cce Jun 26, 2023

Choose a reason for hiding this comment

cce Jun 26, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 16, 2023 • edited Loading

Codecov Report

bbroder-algo left a comment

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

algorandskiy Jun 20, 2023

Choose a reason for hiding this comment

gmalouf commented Jun 20, 2023

algorandskiy commented Jun 21, 2023

winder Jun 16, 2023 •

edited

Loading

gmalouf Jun 16, 2023 •

edited

Loading

codecov bot commented Jun 16, 2023 •

edited

Loading