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

Barista currently is only able to take GET requests, expand to proxy POSTs as well #151

Closed
kltm opened this issue Aug 4, 2015 · 8 comments

Comments

@kltm
Copy link
Member

kltm commented Aug 4, 2015

We ran into this issue soon after we started capturing current individual positions with the graph editor during save events, with the individual position updates overwhelming allowed the GET size.

During the geneontology/bbop-rest-manager rewrite, easy POSTing was added for all of the manager engines; as well, @hdietze believes that Minerva should take POSTs as well (although untested). However, it turns out the Barista (barista.js) was written around only accepting GETs. The idea for this item would be to get Barista passing both GETs and POSTs through transparently. rather than just the current GETs.

Why don't I just do this real quick? This issue is that I do a fair amount of information pealing and URL rewriting in the proxying step (to get and store information or replace some arguments with others); this process already fights how the proxy wants to work a little bit, and there is no simple mapping for the POST part of the problem (although I took a very quick stab at it).

As a temporary workaround, since both Minerva and Barista are under our control, we'll just declare them GET-only servers for the time being (bing!) and try to increase their limits sufficiently for current purposes.

hdietze pushed a commit to geneontology/minerva that referenced this issue Aug 5, 2015
* add conf parameter for buffer sizes
* add test case with long http get request
* see issue: geneontology/noctua#151
@hdietze
Copy link
Contributor

hdietze commented Aug 5, 2015

Updated the minerva code (embedded jetty config) to use 64 k buffer instead of 6k jetty default.
Test case now works for the longer http get

@kltm
Copy link
Member Author

kltm commented Aug 5, 2015

Thank you for the update--I can confirm now that Barista seems happy to pass whatever through for what I've tested and that Minerva will process it and return. This is a sufficient workaround for the time being until a proper POST proxy in Barista can be created.

@kltm
Copy link
Member Author

kltm commented Dec 10, 2015

During the meeting, Val reported an interesting issue, which seems to be able to be broken down into two parts. The second part is that further operations on the model seem to be impossible after a certain point. gomodel:5667fdd400000125

What seems to be happening here is that the request URI from the client is large enough that barista sends a 414. I thought it should be lower than the 80k limit, which seems to be hard: http://stackoverflow.com/questions/26807266/is-it-necessary-to-limit-the-size-of-get-requests-parameter-strings-in-node-js

I'm going to see if I can lift it in express (assuming that we are indeed still under the 80k limit), otherwise, we need to double down and get

tagging @ValWood and @cmungall

@kltm
Copy link
Member Author

kltm commented Dec 10, 2015

Gargh. In the interim, I'm going to go into the repl and try and flush what's there. Either way, a lot of layout information information will be lost (which is one of the reasons for these larger comms packets).

@kltm kltm added this to the 2015-12-push milestone Dec 10, 2015
@kltm
Copy link
Member Author

kltm commented Dec 10, 2015

While there is technically some workarounds, the takeaway is that once models get beyond a certain size, things become unusable. This is now the top priority. I'll use the @ValWood as the testbed. This will involve a lot of restarts of barista, but I'm going to try avoid touching (production) minerva until a fix is in.

hdietze pushed a commit to geneontology/minerva that referenced this issue Dec 16, 2015
hdietze pushed a commit to geneontology/minerva that referenced this issue Dec 16, 2015
kltm added a commit that referenced this issue Dec 19, 2015
kltm added a commit that referenced this issue Dec 19, 2015
…STs as current http proxy is apparently unusable for our purposes, more discussion in tracker; likely almost complete work on #151
kltm added a commit that referenced this issue Dec 19, 2015
@kltm
Copy link
Member Author

kltm commented Dec 19, 2015

Unfortunately, due to problems upstream outside our control, we are unable to do the obvious thing here. In the case of trying to proxy GET requests, we use the (now hated) http-proxy package, taking the incoming request, making a couple of checks against it and changes to it, and then pass it on http-proxy to be taken care of.

However, in the case of POST requests containing bodies, due upstream issues like and surrounding http-party/node-http-proxy#180 , we cannot examine and modify the body and have the proxy work (just hangs, or gives errors, or something) in a reliable and robust way. We tried all of the suggest solutions, and some of our own, with varying degrees of success, but the unpleasant outcome of this is that we are just going have to manually proxy requests ourselves using the http client. This is more complicated and has more overhead, but is more tracable, robust, and we have complete control without fighting with the various middlewares.

In the future, if this works out, we may just switch the GET off of the api_proxy and just do manual for it as well--that will save us some code repetition and make it all a little more readable.

@kltm
Copy link
Member Author

kltm commented Dec 19, 2015

As a temporary workaround in the future if an issue like this comes up, I've added an "emergency" save button under the [Skunkworks] menu. This save function should be able to save your model in almost any circumstances.

@kltm
Copy link
Member Author

kltm commented Dec 21, 2015

This fix has been deployed, and a little testing seems to indicate that it works. It did not fix, however, @ValWood 's issue with gomodel:5667fdd400000125, which I've started in a new issue: #264 .

@kltm kltm closed this as completed Dec 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants