Tolerate existing paging control in SearchWithPaging #51
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -268,6 +268,9 @@ func NewSearchRequest( | |||
| } | |||
| } | |||
|
|
|||
| // SearchWithPaging augments the given search request with a paging control containing the given paging size, then | |||
| // issues a search with buffered paging using the request. All paged responses are buffered so that the result can | |||
| // be returned atomically. | |||
| func (l *Conn) SearchWithPaging(searchRequest *SearchRequest, pagingSize uint32) (*SearchResult, error) { | |||
There was a problem hiding this comment.
This function is really doing two things:
- adding the paging control (which is easy to do yourself, and depending on the calling code, you might want to do yourself. I'd even consider requiring it in a future major version and removing the pagingSize param from here... not a huge fan of mutating the SearchRequest inside the function)
- searching/paging/appending (which is hard to do yourself)
What if we made SearchWithPaging tolerate a searchRequest that already had a matching ControlTypePaging control? That would let us avoid expanding the interface surface area
cases:
ControlTypePagingcontrol missing, adds one matching pagingSize (current behavior)ControlTypePagingcontrol present but not actually a*ControlPaging, returns errorControlTypePagingcontrol present and matching pagingSize, no-op (would enable your use case)ControlTypePagingcontrol present but not matching pagingSize, returns error
There was a problem hiding this comment.
What would I pass through in the third case? SearchWithPaging(mySearchRequest, someIgnoredInt) ?
There was a problem hiding this comment.
you'd pass a pageSize that matched the already-present control's PagedSize
803d130 to
907411e
Compare
907411e to
2edeca7
Compare
|
@liggitt Changed |
|
LGTM. @johnweldon, any feedback on tolerating an already-present paging control like this? |
|
Looks like a nice improvement, and low risk. LGTM |
liggitt
added a commit
that referenced
this pull request
Feb 16, 2016
Tolerate existing paging control in SearchWithPaging
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With the current implementation of
Search, our interaction with this library has been as such:SearchRequestSearchResponseThe current API for
SearchWithPagingdoes not honor this workflow and expects something other than a fully-qualifiedSearchRequestto function. This PR adds another API call that allows for a fully qualified search request to be all that is necessary to issue a request to search with client-side page stitching.@johnweldon @liggitt PTAL