-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[24.0] Fix history export with missing dataset hids #18052
[24.0] Fix history export with missing dataset hids #18052
Conversation
The rest of the test failures seem unrelated. |
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.
I think this is good as-is, and inspected archives before and after and they look good to me.
I think for consistency it probably makes sense to do it across the board as you have done it here, but to toss it out for consideration -- did you think about continuing to use the hid primarily, only utilizing encoded_id when there isn't one? Is there any utility retained there in being able to see the expected ordering by hid at a glance in the filenames?
That is a good point. I didn't think about it when I made the change. But it felt (to me) like the hid was more a way of making sure there were no collisions when writing the files. If you open/extract the archive to inspect the contents I don't know how useful it is to know in which order they were created unless all the files have a similar name and in that case it would probably help to have the I slightly prefer the consistency too, but I'm happy to leave it with the hid and fallback to the |
@davelopez Well, since both of us have the slight preference for consistency, let's stick with that! :) I just wanted to make sure it was considered. |
One more question here. My instance is on 23.2. Do you think that it's a good idea to use these commits on my instance if I want to archive histories? |
I guess we can backport this to 23.2. I will open a PR. |
Fixes #18032
Replaces the dataset
hid
with theencoded_id
for generating filenames.How to test the changes?
License