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

Pipelining followups #147

Merged
merged 33 commits into from
Aug 31, 2023
Merged

Pipelining followups #147

merged 33 commits into from
Aug 31, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Aug 29, 2023

Summary

Addressing the Pipelining Followups Issue along with the "CLI health endpoint bug" which I discovered while working on the block generator.

The CLI Health Endpoint Bug

This bug was actually introduced by the Pipelining PR which preceded the health endpoint, and in particular this line which caused Start() itself to wait forever. As a consequence, the setup of the health endpoint was never reached while conduit was running.

Issue

#141

TODO's

  • remove retriesNoOutput() and its test, unless a use case for this PR comes up.

Testing

Introducing TestHealthEndpoint() in cli_test.go for checking that the health endpoint is ON/OFF as configured. I verified that when the bug is re-introduced, the API_ON case fails as expected. For the purposes of the test, I also introduced a noop importer which waits 100 ms before returning in its GetBlock() method.

conduit/pipeline/common.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #147 (d635841) into master (442791a) will increase coverage by 5.23%.
Report is 57 commits behind head on master.
The diff coverage is 82.80%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   67.66%   72.89%   +5.23%     
==========================================
  Files          32       38       +6     
  Lines        1976     2789     +813     
==========================================
+ Hits         1337     2033     +696     
- Misses        570      646      +76     
- Partials       69      110      +41     
Files Changed Coverage Δ
conduit/data/block_export_data.go 100.00% <ø> (+92.30%) ⬆️
conduit/metrics/metrics.go 100.00% <ø> (ø)
conduit/pipeline/metadata.go 73.52% <ø> (+4.41%) ⬆️
conduit/plugins/config.go 100.00% <ø> (ø)
conduit/plugins/importers/algod/metrics.go 100.00% <ø> (ø)
...gins/processors/filterprocessor/fields/searcher.go 77.50% <ø> (ø)
...ins/processors/filterprocessor/filter_processor.go 83.82% <ø> (+3.54%) ⬆️
...plugins/processors/filterprocessor/gen/generate.go 34.28% <ø> (ø)
conduit/plugins/processors/noop/noop_processor.go 64.70% <ø> (+6.81%) ⬆️
pkg/cli/internal/list/list.go 20.75% <ø> (ø)
... and 22 more

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

Comment on lines -699 to -700

<-p.ctx.Done()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the health endpoint bugfix.

@tzaffi tzaffi requested review from a team, winder, Eric-Warehime, shiqizng and algochoi and removed request for a team August 29, 2023 19:00
}

func (imp *noopImporter) GetBlock(rnd uint64) (data.BlockData, error) {
time.Sleep(sleepForGetBlock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to slow down the pipeline. For example, if you have a no-op for your importer and a file-writer for your exporter this limits the output to 10 blocks per second which is a little easier to manage than without having any sort of brakes on. Another option is to make this configurable, but that didn't strike me as ideal either since I'd like the no-op importer to have no config. LMK.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Approved pending your offer to remove the unused function.

@tzaffi
Copy link
Contributor Author

tzaffi commented Aug 30, 2023

Approved pending your offer to remove the unused function.

d635841

@tzaffi tzaffi added the Bug-Fix PR proposing to fix a bug label Aug 30, 2023
@tzaffi tzaffi merged commit 39c6941 into algorand:master Aug 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug-Fix PR proposing to fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants