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

util/log: use v1 log parser in case it is undefined #68318

Closed
wants to merge 1 commit into from

Conversation

aliher1911
Copy link
Contributor

Previously merge-logs were failing to read file if it doesn't contain
log format config string and no --format command line option was
specified.
This is not convenient as most of logs that require merging are still
from older versions of cockroach. And for newer versions we should be
able to use config string to autodetect.
This diff changes the behaviour to use crdb-v1 format by default.

Release note: None

Fixes #68278

Previously merge-logs were failing to read file if it doesn't contain
log format config string and no --format command line option was
specified.
This is not convenient as most of logs that require merging  are still
from older versions of cockroach. And for newer versions we should be
able to use config string to autodetect.
This diff changes the behaviour to use crdb-v1 format by default.

Release note: None
@aliher1911 aliher1911 requested review from knz and cameronnunez August 2, 2021 10:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Aug 2, 2021

There are two different issues at hand here.

merge-logs were failing to read file if it doesn't contain log format config string and no --format command line option was specified.

The code is meant to auto-detect the format from the information present at the start of log files. Can you show us what example log files you've tried to process which caused an error? I'd rather fix the auto-detection first before introducing a fallback.

Then, separately, you mentioned a "panic" to me before. Regardless of whether there is a fallback, we should get the error checking right. Even without a fallback, there should be no panic. Have you identified where the panic comes from?

@knz
Copy link
Contributor

knz commented Aug 2, 2021

oh I see the linked issue has some repro steps. let's look at this first

@knz knz marked this pull request as draft September 10, 2021 09:46
@aliher1911
Copy link
Contributor Author

Fixed by a more consistent change.

@aliher1911 aliher1911 closed this Oct 1, 2021
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.

Can't use debug merge-logs on logs from 20.2 unless explicitly provide flag
3 participants