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

fix(parsers.avro): Handle timestamp format checking correctly #13855

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

athornton
Copy link
Contributor

@athornton athornton commented Sep 1, 2023

resolves #13854

Made check for invalid timestamp format happen outside the if clause that only fired if the format was the empty string.

@athornton
Copy link
Contributor Author

We might want to dump this one in favor of #13856, which subsumes it (handling the format correctly turned out to be useful for testing that fix).

@srebhan srebhan changed the title Fix timestamp format Init() logic fix(parsers.avro): Handle timestamp format checking correctly Sep 5, 2023
Copy link
Member

@srebhan srebhan 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 the fix @athornton! Just one small comment...
I think we should keep this fix in a separate PR as the other PR also changes the behavior of the metric output and that might be controversial...

@srebhan srebhan self-assigned this Sep 5, 2023
@srebhan srebhan added fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins labels Sep 5, 2023
@athornton athornton requested a review from srebhan September 6, 2023 17:35
@athornton
Copy link
Contributor Author

Updated, test cases changed, review re-requested.

Copy link
Member

@srebhan srebhan 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 to me. Thanks @athornton!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 6, 2023
@srebhan srebhan assigned powersj and unassigned srebhan Sep 6, 2023
@powersj
Copy link
Contributor

powersj commented Sep 7, 2023

I am hesitating hitting merge due to the 386 test failure. Do we really need to increase the timeout or is this branch behind master? Or something else?

@athornton athornton force-pushed the fix/timestamp-config branch from 2a9f5a8 to d73e1b0 Compare September 7, 2023 16:57
@athornton
Copy link
Contributor Author

We're not behind master anymore, and the error seems to indicate we really do need longer:

INFO [config_reader] Config search paths: [./ /go/src/github.com/influxdata/telegraf /go/src/github.com/influxdata /go/src/github.com /go/src /go / /root] 
INFO [config_reader] Used config file .golangci.yml 
INFO [lintersdb] Active 33 linters: [asasalint asciicheck bidichk bodyclose depguard dogsled errcheck errname errorlint exportloopref gocheckcompilerdirectives gocritic goprintffuncname gosec gosimple govet ineffassign interfacebloat lll makezero nakedret nilerr nolintlint prealloc predeclared revive sqlclosecheck staticcheck tenv tparallel unconvert unparam unused] 

Too long with no output (exceeded 10m0s): context deadline exceeded

@powersj
Copy link
Contributor

powersj commented Sep 7, 2023

I find that hard to believe given all you did was modify a switch statement ;)

Looking at master you can see we did not max out the system at all:
https://app.circleci.com/pipelines/github/influxdata/telegraf/17914/workflows/307b4206-1dfd-44cd-94d2-3eaa5e3868a1/jobs/276828/resources

I'm going to land this and hope this is just something strange with the lsst-sqre org.

@powersj powersj merged commit bfbe195 into influxdata:master Sep 7, 2023
@github-actions github-actions bot added this to the v1.28.0 milestone Sep 7, 2023
@athornton
Copy link
Contributor Author

athornton commented Sep 7, 2023

It may well be something strange with lsst-sqre, given that we moved away from CircleCI and probably dropped from a paid tier to their free one (we found GitHub Actions to be a much more pleasant experience).

@athornton athornton deleted the fix/timestamp-config branch September 7, 2023 23:20
athornton added a commit to lsst-sqre/telegraf that referenced this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug plugin/parser 1. Request for new parser plugins 2. Issues/PRs that are related to parser plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avro input plugin timestamp format check in Init() is buggy
3 participants