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

hashed truncated n-tuple tree storage hierarchy spec #16

Merged
merged 12 commits into from
Jan 12, 2021

Conversation

pwinckles
Copy link
Contributor

This is an initial pass at a storage root extension that maps OCFL object identifiers to object root paths by first hashing the identifier. It is similar to @neilsjefferies fixed-length mapping.

I understand that the extension format is still being tweaked and I made some assumptions here about a few things that have yet to be formalized.

Copy link
Member

@awoods awoods 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.

@pwinckles
Copy link
Contributor Author

Looks good.

Thanks for the review @awoods! Do you have an opinion about whether caseMapping should be parameterized or if it should always be lowercase?

@rosy1280 rosy1280 added the draft label Aug 18, 2020
@pwinckles pwinckles added in-review and removed draft labels Aug 18, 2020
@pwinckles pwinckles changed the title initial draft of a hashed truncated n-tuple tree storage hierarchy spec hashed truncated n-tuple tree storage hierarchy spec Aug 18, 2020
@awoods
Copy link
Member

awoods commented Sep 4, 2020

@pwinckles : Given the similarities with the related extensions pr-19, many of the comments there likely apply here as well. Would you mind giving it a look at your leisure?

@pwinckles
Copy link
Contributor Author

pwinckles commented Sep 4, 2020

@pwinckles : Given the similarities with the related extensions pr-19, many of the comments there likely apply here as well. Would you mind giving it a look at your leisure?

Yep, thanks for the thorough read @awoods!

One note about the json format. I still think that using a top level key is a bad idea, and it would be nice if there was more clarity on the reasoning in the issue.

@rosy1280
Copy link
Contributor

@pwinckles if you've addressed the changes, can you dismiss stale reviews and re-request them? also it looks like there are some conflicts can you resolve those?

@pwinckles pwinckles force-pushed the 0003-hashed branch 2 times, most recently from d434f57 to 22134c4 Compare September 15, 2020 13:52
Copy link
Contributor

@zimeon zimeon left a comment

Choose a reason for hiding this comment

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

Reads well to me

@pwinckles
Copy link
Contributor Author

pwinckles commented Nov 9, 2020

Updates:

  • Parameters are now defined using constraints
  • Object IDs are specified as being UTF-8 encoded
  • The location and name of the extension config file is updated to reflect spec changes
  • Added a JSON Schema file

of directory name transparency.

Using this extension, OCFL object identifiers are hashed and encoded as hex
strings. These digests are then divided into _N_ n-tuple segments, which are
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

As long as you considered it.

Now to verify the json-schema...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rewrite it as "encoded as lowercase hex strings"

@pwinckles
Copy link
Contributor Author

@awoods updated the language and added a link to the json schema document.

I also ran the schema against all of the examples in the document using this site: https://www.jsonschemavalidator.net/

awoods
awoods previously approved these changes Dec 11, 2020
@zimeon
Copy link
Contributor

zimeon commented Jan 12, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants