file-plugins: enabling messagepack format by tzaffi · Pull Request #142 · algorand/conduit

tzaffi · 2023-08-18T16:36:09Z

File Plugins: Add Messagepack Format and Default to `msgp.gz`

Based on the extension of the file pattern provided in a file_reader or file_writer plugin, choose:

the Encoding Format (based on .msgp or .json)
whether compression is used (based on .gz)

When no pattern is provided, it defaults to *.msgp.gz

Test Plan

This is all tested in CI including a new integration test conduit/plugins/importers/filereader/fileReadWrite_test.go

codecov · 2023-08-18T16:41:31Z

Codecov Report

Merging #142 (6c7f4c8) into master (442791a) will increase coverage by 3.65%.
Report is 56 commits behind head on master.
The diff coverage is 80.61%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   67.66%   71.31%   +3.65%     
==========================================
  Files          32       37       +5     
  Lines        1976     2761     +785     
==========================================
+ Hits         1337     1969     +632     
- Misses        570      690     +120     
- Partials       69      102      +33

Files Changed	Coverage Δ
conduit/data/block_export_data.go	`100.00% <ø> (+92.30%)`	⬆️
conduit/metrics/metrics.go	`100.00% <ø> (ø)`
conduit/pipeline/metadata.go	`69.11% <ø> (ø)`
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 21 more

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

conduit/plugins/exporters/filewriter/util.go

conduit/plugins/importers/filereader/README.md

conduit/plugins/importers/filereader/test_resources/conduit_data/conduit.yml

…ta/conduit.yml

tzaffi · 2023-08-18T17:28:41Z

conduit/plugins/importers/filereader/test_resources/filereader_blocks/0_block.msgp.gz

These test fixtures were generated using the block generator scenario config.allmixed.small.yml

winder

The actual feature looks fine, nice improvement. I have some misc questions and comments about the refactoring and some potential dead code.

winder · 2023-08-21T13:57:37Z

conduit/pipeline/pipeline.go

 }

-// makeConfig creates a plugin config from a name and config pair.
+// configWithLogger creates a plugin config from a name and config pair.


why rename this? Seems unrelated to the rest of the PR

In order to set up the integration test, I wanted to break-out the pure config generating portion to a new method pluginType.GetConfig(). With this refactoring, it's clearer that this method does 2 things:

sets up the plugin's config

returns a logger that inherits the pipeline's logger setup

These seemed sufficiently unrelated to me that I thought it was worth rename.

However, renaming is not at all crucial.

conduit/plugins/exporters/filewriter/util.go

conduit/plugins/exporters/filewriter/file_exporter_test.go

conduit/plugins/importers/filereader/filereader_test.go

winder · 2023-08-21T14:25:49Z

conduit/plugins/importers/filereader/test_resources/conduit_data/conduit.yml

@@ -0,0 +1,72 @@
+# Log verbosity: PANIC, FATAL, ERROR, WARN, INFO, DEBUG, TRACE
+log-level: INFO


I'd consider making this programmatically, or removing as much as possible so that there's less opportunity for things to become stale.

winder · 2023-08-21T14:30:09Z

conduit/plugins/importers/filereader/filereader.go

+// the genesis file is available __in addition to__ the round 0 block file.
+// This is because the encoding assumed for the genesis is different
+// from the encoding assumed for blocks.
+// TODO: handle the case of a multipurpose file that contains both encodings.


Could you elaborate on this TODO in the comment? As it is, I'm not sure exactly what you have in mind.

Also, the canonical genesis file is json. Maybe we should always write it that way?

What I mean in the TODO is that in principle, it's possible to have a block 0 which includes both the genesis information as well as the block information. But you hint that this may not be such a great idea and we should probably just assume we have a genesis.json since that's canonical. Does it actually make sense to have a round 0? Should we eliminate the concept?

Plan:

remove TODO

file exporter/importer ASSUME genesis.json

continue with block 0 in appropriate encoding

winder · 2023-08-21T14:37:09Z

conduit/plugins/metadata.go

 )
+
+// GetConfig creates an appropriate plugin config for the type.
+func (pt PluginType) GetConfig(cfg data.NameConfigPair, dataDir string) (PluginConfig, error) {


What is this for? I'm having a hard time figuring out where it's used. There are already several patterns for creating test configurations in the other plugin tests, I think it would be better to use/refine one of those rather than adding a new scheme.

I'm only using it in fileReadWrite_test.go, and also in the recently renamed pipelineImpl.configWithLogger(). I can easily recreate the logic in the test and revert this new function. I'll go ahead and do that, but I'm going to keep the added typing in this file to the constants Exporter, Processor and Importer as I believe that was the original intent.

Thanks, yes please revert this and add a test helper if necessary. I think this is unique to the filereader/filewriter e2e test, so it probably doesn't need to be a generally available utility.

e08d93a and e9af0bd

…from fileReadWrite_test.go

conduit/plugins/importers/filereader/fileReadWrite_test.go

conduit/plugins/importers/filereader/filereader_test.go

conduit/plugins/exporters/filewriter/util.go

conduit/plugins/importers/filereader/filereader_test.go

conduit/plugins/exporters/filewriter/util_test.go

Co-authored-by: shiqizng <80276844+shiqizng@users.noreply.github.com>

conduit/plugins/importers/filereader/fileReadWrite_test.go

conduit/plugins/exporters/filewriter/README.md

winder · 2023-08-29T17:27:59Z

conduit/plugins/exporters/filewriter/file_exporter.go

+	genesis := initProvider.GetGenesis()
+	genesisPath := path.Join(exp.cfg.BlocksDir, GenesisFilename)


This was missing before?

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

msgp in file plugins

f2bc1a4

Zeph Grunschlag added 2 commits August 18, 2023 11:48

lint

18af031

remove posErr

ce344d4

tzaffi commented Aug 18, 2023

View reviewed changes

conduit/plugins/exporters/filewriter/util.go Show resolved Hide resolved

tzaffi commented Aug 18, 2023

View reviewed changes

conduit/plugins/exporters/filewriter/util.go Outdated Show resolved Hide resolved

Update conduit/plugins/exporters/filewriter/util.go

e41924b

tzaffi commented Aug 18, 2023

View reviewed changes

conduit/plugins/importers/filereader/README.md Show resolved Hide resolved

tzaffi commented Aug 18, 2023

View reviewed changes

conduit/plugins/importers/filereader/test_resources/conduit_data/conduit.yml Outdated Show resolved Hide resolved

Update conduit/plugins/importers/filereader/test_resources/conduit_da…

b46628f

…ta/conduit.yml

tzaffi changed the title ~~msgp in file plugins~~ file-plugins: enabling messagepack format Aug 18, 2023

tzaffi marked this pull request as ready for review August 18, 2023 17:14

gofmt

b7c82a1

tzaffi requested a review from a team August 18, 2023 17:22

tzaffi commented Aug 18, 2023

View reviewed changes

Zeph Grunschlag added 2 commits August 18, 2023 15:58

change file patterns in README's

b1c2b0b

readmes

5c06330

winder suggested changes Aug 21, 2023

View reviewed changes

Zeph Grunschlag added 8 commits August 21, 2023 14:48

Merge branch 'master' into file-plugins-msgp

ae1ee48

per CR suggestion, comment out configuration not explicitly required …

d7fd948

…from fileReadWrite_test.go

Merge remote-tracking branch 'origin/master' into file-plugins-msgp

1e71d1a

per CR discussion: genesis is always genesis.json

497df82

lint

fafc6d7

per CR discussion: revert renaming/factoring of pipeline's makeConfig()

e08d93a

lint

488f1e8

complete revert

e9af0bd

tzaffi commented Aug 22, 2023

View reviewed changes

conduit/plugins/importers/filereader/fileReadWrite_test.go Outdated Show resolved Hide resolved

tzaffi and others added 2 commits August 22, 2023 16:41

Update conduit/plugins/importers/filereader/fileReadWrite_test.go

873cc83

trim the genesis - ridonculous

85714d1

tzaffi requested a review from winder August 22, 2023 21:50

shiqizng reviewed Aug 25, 2023

View reviewed changes

Zeph Grunschlag and others added 3 commits August 25, 2023 13:21

per CR: remove unneeded assignment

b18d85b

Update conduit/plugins/exporters/filewriter/util.go

27b2168

Co-authored-by: shiqizng <80276844+shiqizng@users.noreply.github.com>

test defaults should actually test the defaults

fb7ea14

tzaffi requested a review from shiqizng August 25, 2023 19:12

typo

14a64cd

shiqizng reviewed Aug 28, 2023

View reviewed changes

conduit/plugins/importers/filereader/fileReadWrite_test.go Show resolved Hide resolved

shiqizng approved these changes Aug 28, 2023

View reviewed changes

winder approved these changes Aug 29, 2023

View reviewed changes

tzaffi and others added 2 commits August 29, 2023 12:33

Update conduit/plugins/exporters/filewriter/README.md

18fb837

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

file not block

6c7f4c8

tzaffi merged commit 1ee2e86 into algorand:master Aug 29, 2023

		@@ -0,0 +1,72 @@
		# Log verbosity: PANIC, FATAL, ERROR, WARN, INFO, DEBUG, TRACE
		log-level: INFO

		genesis := initProvider.GetGenesis()
		genesisPath := path.Join(exp.cfg.BlocksDir, GenesisFilename)

Conversation

tzaffi commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File Plugins: Add Messagepack Format and Default to msgp.gz

Test Plan

Uh oh!

codecov bot commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tzaffi Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tzaffi commented Aug 18, 2023 •

edited

Loading

File Plugins: Add Messagepack Format and Default to `msgp.gz`

codecov bot commented Aug 18, 2023 •

edited

Loading

tzaffi Aug 21, 2023 •

edited

Loading