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

tools: update block-generator to use conduit binary #5306

Merged
merged 7 commits into from Apr 20, 2023

Conversation

shiqizng
Copy link
Contributor

@shiqizng shiqizng commented Apr 14, 2023

#2869. This PR replaces the indexer binary with conduit binary in the runner. It also includes some encoding fixes in the generator.

@@ -312,7 +312,9 @@ func (g *generator) WriteGenesis(output io.Writer) error {
FeeSink: g.feeSink.String(),
Timestamp: g.timestamp,
}
return json.NewEncoder(output).Encode(gen)

_, err := output.Write(protocol.EncodeJSON(gen))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

genesis response needs to be msgp encoded.

output.Write(block)
g.finishRound(numTxnForBlock)
return nil
}
Copy link
Contributor Author

@shiqizng shiqizng Apr 14, 2023

Choose a reason for hiding this comment

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

postgresql exporter adds blocks starting from round 0.

if err != nil {
return err
}
g.ledger.WaitForCommit(basics.Round(g.round))
output.Write(block)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sdk requests the raw block.

RunnerCmd.Flags().StringVarP(&runnerArgs.PostgresConnectionString, "postgres-connection-string", "c", "", "Postgres connection string.")
RunnerCmd.Flags().DurationVarP(&runnerArgs.RunDuration, "test-duration", "d", 5*time.Minute, "Duration to use for each scenario.")
RunnerCmd.Flags().StringVarP(&runnerArgs.ReportDirectory, "report-directory", "r", "", "Location to place test reports.")
RunnerCmd.Flags().StringVarP(&runnerArgs.LogLevel, "log-level", "l", "error", "LogLevel to use when starting Indexer. [error, warn, info, debug, trace]")
RunnerCmd.Flags().StringVarP(&runnerArgs.LogLevel, "log-level", "l", "ERROR", "LogLevel to use when starting Indexer. [PANIC, FATAL, ERROR, WARN, INFO, DEBUG, TRACE]")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating these to match log-level supported by conduit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does conduit fail if the casing is wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like lower case also works. I'll revert this change.

@shiqizng shiqizng marked this pull request as ready for review April 14, 2023 20:04
Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Looks good. Just a couple of nits.

tools/block-generator/runner/run.go Outdated Show resolved Hide resolved
tools/block-generator/data/conduit.yml Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@ help() {
echo " -r|--report-dir directory where the report should be written."
echo " -d|--duration test duration."
echo " -l|--level log level to pass to Indexer."
echo " -g|--generator use a different indexer binary to run the generator."
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's no longer part of the indexer binary, this should be required. Mark as required on line 79

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

--algod-net localhost:11111 \
--algod-token security-is-our-number-one-priority \
--metrics-mode VERBOSE \
-P "host=localhost user=algorand password=algorand dbname=generator_db port=15432 sslmode=disable"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this script to work for conduit, we would need to set up a configuration conduit config file. I think we can remove this script since run_runner and run_tests are running the same setup using runner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be ok with deleting the other scripts too.

// check /metrics endpoint is available before running the test
var resp *http.Response
for retry := 0; retry < 10; retry++ {
resp, err = http.Get(fmt.Sprintf("http://%s/metrics", metricsNet))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that '/metrics' isn't always available by the time runTest starts. this causes block-generator to exit with error problem collecting metrics (1 / 3.003180017s): unable to read metrics url 'http://localhost:9999/metrics'.
I'm adding these lines to make sure /metrics is available before running tests.

@shiqizng shiqizng requested review from winder and tzaffi April 19, 2023 19:00
@winder
Copy link
Contributor

winder commented Apr 20, 2023

In a followup PR could you do a search/replace for Indexer and indexer to Conduit?

// create config file in the right data directory
f, err := os.Create(path.Join(dataDir, "conduit.yml"))
if err != nil {
return fmt.Errorf("creating conduit.yml: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("creating conduit.yml: %v", err)
return fmt.Errorf("creating conduit.yml: %w", err)

nit for consistency with other error wraps here


err = t.Execute(f, conduitConfig)
if err != nil {
return fmt.Errorf("execute template file: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("execute template file: %v", err)
return fmt.Errorf("execute template file: %w", err)

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

LGTM

@tzaffi
Copy link
Contributor

tzaffi commented Apr 20, 2023

In a followup PR could you do a search/replace for Indexer and indexer to Conduit?

I'm doing this for the README. Stay tuned.

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.

None yet

3 participants