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

add a Size field to Query's Result #134

Merged
merged 5 commits into from
Dec 2, 2019
Merged

Conversation

MichaelMure
Copy link
Contributor

Rational is that some datastore can easilty get the size of a value when listing only keys, so we might as well have a way to pass that. One way this could be useful is when implementing a cache for Has/GetSize.

query/query.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

Hm. I just wanted this when messing with ipfs repo stat (I wanted to be able to report sizes for various record types).

@MichaelMure
Copy link
Contributor Author

I changed the semantic so that:

  • the calling side can opt-in to always have the size set, even with KeysOnly, even it that implies a performance cost
  • a datastore implementation can optionally always return the size if it doesn't imply a performance cost

I feel like that the best compromise. What do you think ?

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM!

@Stebalien
Copy link
Member

Stebalien commented Oct 6, 2019

Could you file pull requests against the other datastores before we merge this?

  • go-ds-badger
  • go-ds-bolt broken
  • go-ds-flatfs
  • go-ds-leveldb
  • go-ds-redis
  • go-ds-s3

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Actually, this needs to update the mount datastore. We don't need to forward the RequestSize to child queries.

While we're at it, we should add tests to test/suite.go.

@MichaelMure
Copy link
Contributor Author

Actually, this needs to update the mount datastore. We don't need to forward the RequestSize to child queries.

Not sure I follow, It doesn't at the moment, but it should, right ?

@Stebalien
Copy link
Member

Stebalien commented Oct 21, 2019 via email

MichaelMure and others added 5 commits November 22, 2019 13:36
Rational is that some datastore can easilty get the size of a value when
listing only keys, so we might as well have a way to pass that. One way this
could be useful is when implementing a cache for Has/GetSize.
@MichaelMure
Copy link
Contributor Author

  • fixed the mount datastore
  • renamed ReturnsSize to ReturnsSizes
  • added a test for ReturnsSizes

Should be good to go now

@MichaelMure
Copy link
Contributor Author

MichaelMure commented Nov 22, 2019

Could you file pull requests against the other datastores before we merge this?

@Stebalien
Copy link
Member

Unfortunately, I can't merge this until we actually have PRs for those issues. Otherwise, all those datastores will return invalid results.

@MichaelMure
Copy link
Contributor Author

Any chance the respective maintainers could help here ?

@Stebalien
Copy link
Member

Unfortunately, I am the de facto maintainer.

@MichaelMure
Copy link
Contributor Author

I did a pass on all of them but go-ds-bolt is problematic as it's broken at the moment.

@Stebalien Stebalien merged commit d4417ca into ipfs:master Dec 2, 2019
@Stebalien
Copy link
Member

All merged and updated.

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.

2 participants