hpc / mpifileutils

File utilities designed for scalability and performance.
https://hpc.github.io/mpifileutils
BSD 3-Clause "New" or "Revised" License
168 stars 66 forks source link

daos: update setting of container label #488

Closed dsikich closed 3 years ago

dsikich commented 3 years ago

Remove copying of container label when copying from DAOS->DAOS. The container label in DAOS is now required to be unique. The container label is still serialized for now until the API to check if container label already exists in DAOS is available for use.

Only set container label on deserialization if it does not exist.

Fix bug in cont_deserialize_all_props where a copy of the pointer was being passed to it instead of the address to the pointer, causing the properties to not be set correctly in cont_create.

Signed-off-by: Danielle Sikich danielle.sikich@intel.com

dsikich commented 3 years ago

@daltonbohning the container open by label API has been merged, so I can add a check to this PR that only deserializes the container label of the serialized container if it doesn't already exist, and same for the preserve option that was already merged.

daltonbohning commented 3 years ago

@daltonbohning the container open by label API has been merged, so I can add a check to this PR that only deserializes the container label of the serialized container if it doesn't already exist, and same for the preserve option that was already merged.

@dsikich Sounds good! Something I'm not sure about that will need to be verified is how to know whether the cont_open failed because the UUID already exists, or because the label already exists.

dsikich commented 3 years ago

@daltonbohning the container open by label API has been merged, so I can add a check to this PR that only deserializes the container label of the serialized container if it doesn't already exist, and same for the preserve option that was already merged.

@dsikich Sounds good! Something I'm not sure about that will need to be verified is how to know whether the cont_open failed because the UUID already exists, or because the label already exists.

@daltonbohning I don't think that should be an issue right now because we always generate a new UUID for deserialized containers currently?

daltonbohning commented 3 years ago

@dsikich Sounds good! Something I'm not sure about that will need to be verified is how to know whether the cont_open failed because the UUID already exists, or because the label already exists.

@daltonbohning I don't think that should be an issue right now because we always generate a new UUID for deserialized containers currently?

@dsikich Right, so I guess it's okay for now, but something we'll need to think about later?

dsikich commented 3 years ago

@dsikich Sounds good! Something I'm not sure about that will need to be verified is how to know whether the cont_open failed because the UUID already exists, or because the label already exists.

@daltonbohning I don't think that should be an issue right now because we always generate a new UUID for deserialized containers currently?

@dsikich Right, so I guess it's okay for now, but something we'll need to think about later?

@daltonbohning yeah, that is true.

dsikich commented 3 years ago

@daltonbohning I have updated this to check if the label already exists in the pool when deserializing a container. If the label doesn't exist then it sets it, and if it does exist then no label is set. There was also a bug in cont_deserialize_all_props where the properties were not getting set before passing to cont_create because a copy of a pointer was being passed to a function. They should be set correctly now.

adammoody commented 3 years ago

Thanks @dsikich and @daltonbohning . This looks good to me. One suggestion is that it would be helpful to add a comment in the code to explain why the label is being handled differently than the other properties. Maybe just copy and paste your PR description (or a shorter version of it) somewhere as code comment? Good to merge after that. Thanks!

dsikich commented 3 years ago

Thanks @dsikich and @daltonbohning . This looks good to me. One suggestion is that it would be helpful to add a comment in the code to explain why the label is being handled differently than the other properties. Maybe just copy and paste your PR description (or a shorter version of it) somewhere as code comment? Good to merge after that. Thanks!

@adammoody thanks! added some extra comments