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

Add Truncate and ReadOnly options for badger #1842

Merged
merged 2 commits into from
Oct 16, 2019

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Oct 7, 2019

Which problem is this PR solving?

Short description of the changes

  • Command line options --badger.truncate and --badger.read-only are passed to badger.Open()

@burmanm
Copy link
Contributor Author

burmanm commented Oct 7, 2019

travis failures seem to have nothing to do with this PR:

FAIL github.com/jaegertracing/jaeger/cmd/ingester/app/consumer 3.038s

Or crossdock for that matter.

@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #1842 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1842      +/-   ##
=========================================
- Coverage   98.44%   98.4%   -0.04%     
=========================================
  Files         197     197              
  Lines        9631    9645      +14     
=========================================
+ Hits         9481    9491      +10     
- Misses        114     117       +3     
- Partials       36      37       +1
Impacted Files Coverage Δ
plugin/storage/badger/factory.go 98.13% <100%> (+0.03%) ⬆️
plugin/storage/badger/options.go 100% <100%> (ø) ⬆️
cmd/query/app/static_handler.go 83.33% <0%> (-3.51%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e9b18d...0eae06f. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@objectiser
Copy link
Contributor

@burmanm Wondering if there is an automatic approach that could be used to overcome the issue - rather than having to specific command line options?

Specifically thinking about when used with the operator, what would prompt the operator to restart the all-in-one server with these options? Is there an "auto-healing" approach that could be used instead to automatically recover (if optionally enabled)?

@burmanm
Copy link
Contributor Author

burmanm commented Oct 8, 2019

@objectiser Well, for readonly there's no "automatic" as it's something someone might want, like for example reading a backup/snapshot from past.

As for the truncate, it shouldn't be needed ever, but for some reason that's been the case. I don't think truncating a WAL log automatically is a good thing since stuff will be lost that might be recoverable (and we shouldn't make the decision). Maybe it's a bug in the version of badger we ship with Jaeger, it would have been good to know more details or if there's a reproducer. Maybe the filesystem corrupted the file on close for example.

@pavolloffay
Copy link
Member

As for the truncate, it shouldn't be needed ever, but for some reason that's been the case. I don't think truncating a WAL log automatically is a good thing since stuff will be lost that might be recoverable (and we shouldn't make the decision).

Could we truncate automatically in badger storage if badger fails to start with a specific error? Can be the data recovered in any other way?

@burmanm
Copy link
Contributor Author

burmanm commented Oct 9, 2019

Well, the error isn't typed, but we could try parsing the returned error message. Assuming it doesn't change in some version..

@objectiser
Copy link
Contributor

@burmanm @jpkrohling @pavolloffay Although I started the conversation regarding whether auto recovery would be possible, wondering if better to get this merged and then look at whether something more automated could be achieved in a separate PR, or possibly in the operator?

@jpkrohling
Copy link
Contributor

+1 from me, @objectiser

@objectiser objectiser merged commit d77a9b5 into jaegertracing:master Oct 16, 2019
radekg pushed a commit to Klarrio/jaeger that referenced this pull request Oct 28, 2019
…aegertracing#1832 (jaegertracing#1842)

Signed-off-by: Michael Burman <yak@iki.fi>
Signed-off-by: radekg <radek@gruchalski.com>
@yurishkuro yurishkuro added this to the Release 1.15 milestone Nov 7, 2019
backjo pushed a commit to backjo/jaeger that referenced this pull request Dec 19, 2019
…aegertracing#1832 (jaegertracing#1842)

Signed-off-by: Michael Burman <yak@iki.fi>
Signed-off-by: Jonah Back <jonah@jonahback.com>
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.

5 participants