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

Revive REST functionality with what conditions? #126

Open
robinp-tw opened this issue Dec 18, 2020 · 6 comments
Open

Revive REST functionality with what conditions? #126

robinp-tw opened this issue Dec 18, 2020 · 6 comments

Comments

@robinp-tw
Copy link

robinp-tw commented Dec 18, 2020

Hello,

I found there was REST search functionality that was deleted in 7444844. I find that functionality useful, so would like to restore it.

Can it be restored as-is (maybe with some tweaks, to support result size limit from request etc), or do you have any concerns with it? Thank you!

@hanwen
Copy link
Contributor

hanwen commented Dec 21, 2020

The REST support was removed because I added it to support some other functionality which never panned out.

What do you want to do? I see two options:

  1. Make the core API available over HTTP / JSON.

  2. add a &format=JSON parameter to the Search endpoint in server.go, and dump the data in the web API format.

my main concern is that I don't want to have an extra API flavor, because that necessarily will create extra conversion code that needs to be maintained.

Maybe the srcgraph folks have ideas/preferences too. Summoning @keegancsmith

@keegancsmith
Copy link
Contributor

Maybe the srcgraph folks have ideas/preferences too. Summoning @keegancsmith

We use "net/rpc" to remotely expose a "zoekt.Searcher". We originally used the json API, but quickly moved to something which can expose the core API. What about option 2, but directly marshalling the response from Searcher.Search into JSON?

FYI we have been working on a streaming API internally, and will likely add an adapter to zoekt.Searcher which allows use to do streaming RPCs. I wasn't originally thinking this would be that useful to upstream, but if you feel otherwise we can debate that decision :)

@hanwen
Copy link
Contributor

hanwen commented Jan 13, 2021

Is the streaming just for the browser <-> server communications? You'd need all results anyway to sort the results, right?

@keegancsmith
Copy link
Contributor

Is the streaming just for the browser <-> server communications?

browser <-> server. But we also want to do streaming between services so as to return the results sooner. We have use cases where a search can take a long time. For example a poor regular expression over a large cluster can take a while to return.

You'd need all results anyway to sort the results, right?

We currently aren't relying on ranking/sorting, so no. When we do start doing this, we will try and visit shards/repos in a smart order (somewhat like the rankedShard idea in zoekt).

@hanwen
Copy link
Contributor

hanwen commented Jan 13, 2021

might make sense to add an option "Unsorted" to the SearchRequest, then making all of the APIs streaming makes more sense.

@kisabaka
Copy link

@hanwen @keegancsmith
JSON marshalling seems to be a simple change: kisabaka/zoekt@6b51c3c

Let me know if you're happy with that approach

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

No branches or pull requests

4 participants