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

siva: reload metadata #84

Merged
merged 1 commit into from
Aug 20, 2019

Conversation

mcarmonaa
Copy link
Contributor

Closes #82

@mcarmonaa mcarmonaa requested a review from jfontan August 13, 2019 14:51
siva/library.go Outdated
// not provided or already in metadata.
DontGenerateID bool
// Metadata flags if the library must use stored metadata.
StoredMetadata bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Loading metadata should be the default behavior. I would enforce it for now and if needed we can add a DisableMetadata option when we find the use case.

siva/metadata.go Outdated
type Version struct {
// Offset is a position in the siva file.
// Bookmark represents a valid siva file point to read from.
type Bookmark struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the rationale for changing Version to Bookmark? It feels more natural that calling .Version() returns a Version and not a Bookmark.

@mcarmonaa mcarmonaa force-pushed the improvement/siva-reload-metadata branch from 155dd11 to 782580c Compare August 20, 2019 10:29
@mcarmonaa
Copy link
Contributor Author

@jfontan I did the changes:

  • Metadata is always used. By default metadata files are created if not exists. Also if there's no previous metadata for a library and you instantiate it with an empty id, an uuid will be generated.

  • There is a MetadataReadOnly option that doesn't allow to the library create or modify metadata. If you instantiate a library with this mode enable, non id is generated if you pass an empty string and there's no previous metadata.

@mcarmonaa mcarmonaa requested a review from jfontan August 20, 2019 10:33
siva/library.go Outdated

if metadata != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be if metadata != nil && id == "" {? There's the case where you provide an ID but there's metadata. Shouldn't we use the provided p id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that is the default case we want, yes it has to be changed.

so the default would be overwrite the id in case the given one differs from the metadata id, what would we do with MetadataReadOnly in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case there's no write unless a save operation is explicitly called and I believe SaveMetadata was deleted.

Copy link
Contributor

@jfontan jfontan left a comment

Choose a reason for hiding this comment

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

LGTM, just check the comment to see if it makes sense.

Signed-off-by: Manuel Carmona <manu.carmona90@gmail.com>
@mcarmonaa mcarmonaa force-pushed the improvement/siva-reload-metadata branch from 782580c to b4407c0 Compare August 20, 2019 15:37
@jfontan jfontan merged commit 0972e27 into src-d:master Aug 20, 2019
@mcarmonaa mcarmonaa deleted the improvement/siva-reload-metadata branch August 20, 2019 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[siva] reload metadata when changed
2 participants