-
Notifications
You must be signed in to change notification settings - Fork 544
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
fixed the issue that windows cannot contain filenames with colons (replaced ':' with '+') #1700
Conversation
…eplaced ':' with '#')
Hi, thanks for fixing this. We were very surprised to hear that Tempo doesn't work on Windows, since that file naming hasn't changed recently, it must have been broken for a long time or never worked. I agree with the overall direction that we want to change the naming convention of the wal files but I'd like to avoid OS-specific logic and have Tempo run the same everywhere. That changes the scope of this PR quite a bit, because we'd need to be backwards-compatible and parse existing files with the old separator (and new files are written with the new separator). Also, would like to find an alternative to |
I am coming from the problem of configuring Tempo on Windows. Based on the concerns stated above, I think a file name with And, please make it go asap, it's not working on Windows and you know how prioritized it should be. |
No, At the risk of bike-shedding, how about
Finally, @kilian-kier are you interested in doing the non-OS specific approach on this PR? We would change all wal file names to the new separator, but check for |
Yes i can do it. Is it enough just to look if there is a : in the filename and if not it should check for +? |
Great. Yes that sounds good, but reverse the order to check for the new separator |
@mdisibio it should work now |
@kilian-kier There some tests that need to be updated for the new separator. Here is an example. To run the tests locally execute
|
@mdisibio ok finally also the tests should work |
# Conflicts: # CHANGELOG.md # tempodb/wal/append_block.go # tempodb/wal/append_block_test.go # tempodb/wal/wal.go
Hi @kilian-kier. We recently updated |
@mapno Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kilian-kier Thanks a lot, great to see Windows-support fixed.
Hi there! It does not work for me ( I've built main branch from sources using linux VM: I'm very new with Go, doing somthing wrong? Still getting this
I'm using this config:
Regards. |
Did you resolve this prob? |
try this : storage: |
What this PR does:
It saves wal files with '+' filename separator, instead of ':' , because Windows does not allow it
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]