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

Integrate search v2 #1082

Merged
merged 13 commits into from
Feb 21, 2025
Merged

Integrate search v2 #1082

merged 13 commits into from
Feb 21, 2025

Conversation

smallhive
Copy link
Contributor

Closes #1081

@smallhive smallhive marked this pull request as ready for review February 19, 2025 10:55
@roman-khimov
Copy link
Member

Replaces #1078?

@@ -65,6 +65,18 @@ type (
Headers map[string]string
}

// ObjectListResponseContent holds response data for object listing.
ObjectListResponseContent struct {
Copy link
Member

Choose a reason for hiding this comment

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

What are the uses for ObjectInfo? Can this be embedded into ObjectInfo? Do we need both or just one is sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ObjectInfo in general appears with GetExtendedObjectInfo method to get full info about object. Moreover, it contains all object attributes, which are used for responses and other data manipulations

ObjectListResponseContent is a shorter version, which is enough to show a bucket listing

cthulhu-rider and others added 6 commits February 21, 2025 10:43
These fields have never been read since they was added in
47b560e.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
No longer need additional type also.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Neither TODO nor understanding why it might be needed.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 1081-integrate-search-v2 branch from 8cd5c2c to 5c54734 Compare February 21, 2025 10:25
@smallhive smallhive force-pushed the 1081-integrate-search-v2 branch from 5c54734 to ab35807 Compare February 21, 2025 11:20
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Continuation token is not a OID any more.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
Closes #1081.

Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 1081-integrate-search-v2 branch from ab35807 to d0a165b Compare February 21, 2025 13:11
Signed-off-by: Evgenii Baidakov <evgenii@nspcc.io>
@smallhive smallhive force-pushed the 1081-integrate-search-v2 branch from d0a165b to 17d951f Compare February 21, 2025 13:13
}

opts.SetCount(uint32(maxKeys))

filters.AddFilter(object.AttributeFilePath, prefix, object.MatchCommonPrefix)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how prefix here...

searchResults = append(searchResults, psr)
}

sortFunc := func(a, b prefixSearchResult) int {
Copy link
Member

Choose a reason for hiding this comment

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

...works with this sorting function. It's perfectly fine for objects with the same FilePath, but otherwise they're just different objects as per S3. Consider prefix "a" and "ab", "ac" objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is used only for bucket listing. The prefix helps to filter similar objects or just list a "dir". There may be many objects with the same FilePath.
This sorting allows us to find the actual version of the object. But, maybe for this exact place, such multi-level sorting is too much. The question requires separate checks

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect this sorting to be prefix first and time second. Right now IIUC in a/ab/ac situation you can get any order even though technically everyone expects exactly "a, ab, ac". Then if there are multiple versions of "a", those should be sorted according to time stamps.

prmSearch.Filters.AddFilter(object.AttributeFilePath, objectName, object.MatchStringEqual)
filters.AddFilter(object.AttributeFilePath, objectName, object.MatchStringEqual)
} else {
filters.AddFilter(object.AttributeFilePath, "", object.MatchCommonPrefix)
Copy link
Member

Choose a reason for hiding this comment

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

Can this ever really happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least one place exists in code, where we call with an empty object name - on the bucket removing. Any other place with user input may be involved as well

Copy link
Member

Choose a reason for hiding this comment

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

Bucket removal retrieves all objects from bucket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't remove a bucket with objects. In out case with regular objects

Copy link
Member

Choose a reason for hiding this comment

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

OK, but to check we need just a single result, not the whole list, is it taken into account anywhere?

@roman-khimov roman-khimov merged commit 30f920c into master Feb 21, 2025
12 of 18 checks passed
@roman-khimov roman-khimov deleted the 1081-integrate-search-v2 branch February 21, 2025 15:24
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.

Integrate search v2
3 participants