Conversation
hartig
left a comment
There was a problem hiding this comment.
Looks generally very good. Just a few comments.
...rc/main/java/se/liu/ida/hefquin/federation/access/impl/reqproc/RESTRequestProcessorImpl.java
Outdated
Show resolved
Hide resolved
hefquin-base/src/main/java/se/liu/ida/hefquin/base/shared/http/HttpClientProvider.java
Outdated
Show resolved
Hide resolved
hefquin-base/src/main/java/se/liu/ida/hefquin/base/shared/http/HttpClientProvider.java
Outdated
Show resolved
Hide resolved
hefquin-base/src/main/java/se/liu/ida/hefquin/base/shared/http/HttpClientProvider.java
Outdated
Show resolved
Hide resolved
hefquin-base/src/main/java/se/liu/ida/hefquin/base/shared/http/HttpClientProvider.java
Show resolved
Hide resolved
I honestly have no idea. The whole GraphQL connector was written by a student under the supervison of @scferrada . I didn't look so closely at that code, but I remember that it is generally not as well-designed as the core parts of the engine. Using it for real would require quite a bit of additional work. In this sense, if you managed to make
I'm afraid not. But maybe @scferrada can provide some insight. He wrote the Neo4j connector. If I remember correctly, for these tests, he simply used an out-of-box Neo4j server with one of their demo databases.
As I mentioned over there, I would like Something else that just comes to my mind: |
…/HttpClientProvider.java Co-authored-by: Olaf Hartig <olaf.hartig@liu.se>
Absolutely. Changelog updated in cebeb7b. |
hartig
left a comment
There was a problem hiding this comment.
Thanks a lot for addressing my comments!
The PR is ready to merge now.
Notice that I have removed issue #494 as being resolved by this PR. The remaining TODO still to be addressed in a follow-up PR is to reverse the dependency between hefquin-access and hefquin-pgconnector/hefquin-graphqlconnector, and then to move HttpClientProvider to hefquin-access. Can you please give this a try based on what I have outlined in the issue.
This PR add support for HTTP connection reuse in:
SPARQLRequestProcessorImplTPFRequestProcessorImplBRTPFRequestProcessorImplRESTRequestProcessorImplNeo4jRequestProcessorImplGraphQLRequestProcessorImplFor all request types except
GraphQLRequestProcessorImplthe changes are minor, since the implementations for the connections were already built onjava.net.http.HttpClient.For
GraphQLRequestProcessorImpl, there are more substantial changes inGraphQLConnection. During testing with a public endpoint I noticed that therawkey caused exceptions so removed it for now. (What is/was the purpose of this key originally? Is it related to Apollo and GraphiQL somehow?)Btw, is there any documentation on how to setup the servers (and the data needed) for
GraphQLRequestProcessorImplTestandNeo4jRequestProcessorImplTest?The
HttpClientProvideris currently inhefquin-base, but we may wish to move it before merge according to the ungoing discussion in #494.