Skip to content

[GOAL2-614] Updated algoh to capture algod logs on early termination#40

Merged
Karmastic merged 2 commits intoalgorand:masterfrom
egieseke:goal2-614-algoh-report-restart-issues
Jun 20, 2019
Merged

[GOAL2-614] Updated algoh to capture algod logs on early termination#40
Karmastic merged 2 commits intoalgorand:masterfrom
egieseke:goal2-614-algoh-report-restart-issues

Conversation

@egieseke
Copy link
Copy Markdown
Contributor

Please review this change to capture logs In case algod terminates before block watcher initializes.

…ates before block watcher is initialized.

Fix code analysis warnings.
@egieseke egieseke requested review from Karmastic and algobolson June 17, 2019 17:14
Comment thread cmd/algoh/main.go Outdated
genesisID, err := nc.GetGenesisID()
if err != nil {
fmt.Fprintln(os.Stdout, "error loading telemetry config", err)
_, _ = fmt.Fprintln(os.Stdout, "error loading telemetry config", err)
Copy link
Copy Markdown
Contributor

@Karmastic Karmastic Jun 18, 2019

Choose a reason for hiding this comment

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

What's the purpose of the "_, _"? Were you getting lint errors or something? I think we're fine sticking with our standard convention of not doing this - unless there's a reason we need to start doing this, please don't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, Goland required to get the green check for code analysis. I will remove.

Copy link
Copy Markdown
Contributor

@Karmastic Karmastic left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this. A few requests.

Comment thread cmd/algoh/main.go Outdated

// capture logs if algod terminated prior to blockWatcher starting
if !blockWatcherInitialized && algohConfig.UploadOnError {
if errorOutput.output != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a duplicate of the code below - please consolidate. Also - this block should not all be gated by 'UploadOnError' - that should only affect whether we call sendLogs(), like below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK,Refactored into a separate method collectErrorLogs(). Removed UploadOnError condition.

@egieseke egieseke requested a review from Karmastic June 18, 2019 18:10
@egieseke egieseke dismissed Karmastic’s stale review June 19, 2019 20:51

Consolidated error logging as recommended, please re review.

Copy link
Copy Markdown
Contributor

@Karmastic Karmastic left a comment

Choose a reason for hiding this comment

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

Thanks!

@Karmastic Karmastic merged commit 1e099cf into algorand:master Jun 20, 2019
pzbitskiy pushed a commit to pzbitskiy/go-algorand that referenced this pull request Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants