Skip to content

Various config and default setting fixes #866

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

Merged
merged 10 commits into from
May 12, 2020
Merged

Various config and default setting fixes #866

merged 10 commits into from
May 12, 2020

Conversation

tobim
Copy link
Member

@tobim tobim commented May 11, 2020

No description provided.

tobim added 6 commits May 11, 2020 16:54
This inverts the logic to suppress the to file logging of client
commands. The old algorithm overwrites the verbosity setting with
the default value after it was parsed, if the command is either
`vast start` or `vast -N ...`. This means that user specified
levels from the config file or the command line would have no
effect.

The new method does the opposite: Once we know the command does
not start a node the file logging is turned off.
@tobim tobim added the bug Incorrect behavior label May 11, 2020
@tobim tobim requested a review from a team May 11, 2020 21:16
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

The changes you made look fine—thanks!—, but are reliably failing some unit tests. This must be investigated. Hoping it's not too deeply related to Arrow.

Can you also add a changelog entry for the default table slice fix?

@@ -306,19 +306,22 @@ logger {

; Configures the minimum severity of messages written to the log file.
; Possible values: quiet, error, warning, info, verbose, debug, trace.
;file-verbosity = "debug"
; File logging is only available for commands that start a node (e.g.,
; vast start). The levels above 'verbose' are usually not available in
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but intuitively I'd say s/above/below/?

Copy link
Member Author

Choose a reason for hiding this comment

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

info -> low verbosity; debug -> high verbosity?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but info -> higher importance, debug -> lower importance.

But I'll let you decide, this was only a side comment.

@tobim
Copy link
Member Author

tobim commented May 12, 2020

Sorry for the late addition, please check if the last commit is to your liking.

@tobim
Copy link
Member Author

tobim commented May 12, 2020

I just pushed once more to fix the broken formatting in make_command_factory(), no need to re-review the commit.

@tobim tobim merged commit f932d46 into master May 12, 2020
@tobim tobim deleted the story/ch16448 branch May 12, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants