Conversation
To use relative urls users need to:
* Create named virtual chunk containers
* When they add virtual references use locations with protocol `vcc://`
* Make the hostname in the url equal to some VCC name
* If validate_container=True when they set a virtual ref, and
the url is `vcc://foo/bar.nc`, `foo` must match some VCC name.
The final url will be `{foo url prefix}/bar.nc`
TomNicholas
left a comment
TomNicholas
left a comment
There was a problem hiding this comment.
This looks great - I only have one substantive Q (about allowing multiple VCCs with the same name). I think this should meet our requirements.
It would also be nice to have at least like 1 line of docs about this somewhere, now that name actually does something!
| } | ||
|
|
||
| /// Parse a location that may be either `vcc://` relative or an absolute URL. | ||
| pub fn from_url(path: &str) -> Result<VirtualChunkLocation, VirtualReferenceError> { |
There was a problem hiding this comment.
This method is just for API backwards compatibility?
There was a problem hiding this comment.
basically, yes, but it should work for normal and vcc
| } else if scheme == "vcc" { | ||
| return Err(VirtualReferenceErrorKind::NoContainerForName(path.into()).into()); |
There was a problem hiding this comment.
I'm confused - how does ::from_url not also trigger this Err?
There was a problem hiding this comment.
If i understand your question correctly: Because you only get here if the url doesn't have a host, which in the case of vcc:// is expected to be the container name
icechunk/src/config.rs
Outdated
| // Check name uniqueness: if the new container has a name, no existing | ||
| // container with the same name should have a different url_prefix |
There was a problem hiding this comment.
I don't understand how its okay to have two containers with the same name at all. How does IC know which VCC to resolve to?
There was a problem hiding this comment.
Okay actually this is just for updating. But it's still confusing - when I overwrite an existing container by setting a new container with the same name as the old one, is that meant to be an overwrite (in which case there should be no restrictions), or are you trying to force me to delete the old container entirely before I can set the new one (in which case two containers with the same name should not be allowed ever).
There was a problem hiding this comment.
So the issue here is that we have two identities for VCCs and they both need to work for different cases. The url prefix and the name. I changed the approach: if the container you pass here has a name it will be used as the key and replace any other of the same name. If it doesn't have a name, it will use the url prefix as key.
| Default::default(), | ||
| ); | ||
|
|
||
| let result = resolver.expand_location("vcc://unknown/chunk.nc"); |
There was a problem hiding this comment.
Might also be worth testing case of
"vcc://chunk.nc"
where there is no name, but it's using a relative protocol.
|
Thanks for the thorough review @TomNicholas , I implemented what you asked for, and I think set_virtual_container is much more intuitive now. I'll merge once tests pass. |
2 participants
To use relative urls users need to:
vcc://vcc://foo/bar.nc,foomust match some VCC name. The final url will be{foo url prefix}/bar.nc