Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I'm fine if you would merge this and f you'd like to close 576 for the
release. I will work on the docs separately.
---
If your project is set up for it, you can reply to this email and have your
reply
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
That `http_op_3` commit
https://github.com/apache/jena/commit/944e5208ff0fbb4e87fcab7417b760252d9aabfc
did come over into this PR as commit
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
The state is that it builds and passes all tests. The dependency is
upgraded and the use of the deprecated API from HTTP Commons is removed (at
least all that I could find). I now see that I didn't push
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I don't know what state the PR is in. I don't see the changes I made today
to sort things out on the PR so I am unclear what "it" is and what your happy
comment refers to.
I can merge it,or you
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
For my money, we can merge this. I think we might want to keep JENA-576
open until @rvesse is happy with the new documentation.
---
If your project is set up for it, you can reply to this email and
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
@ajs6f What's the next step?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
_O frabjous day! Callooh! Callay!_
Full green light build. Thanks enormously for this help, @afs. I'm still
very much a beginner with the various form of test machinery in the codebase.
After
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
http_op_3 branch should be good now : ServerCtl moved to fuseki1 and used
uniformly. After this is finished, it coudl be cleaned up or more reasonably,
extract that code, add the capabilities to
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
It looks like only a partial set of changes from http_op_3. Maybe I got the
remote repo into a inconsistent state. I pushed some (I hoped) unrelated
cleanup today.
I will now also backport
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
The errors I get in `TS_Fuseki` are always in `TestSPARQLProtocol` or
`TestQuery`, which use `resetServer()` and load up sample data over which to
work. They are `Connection reset` exceptions, oddly.
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
There is a strange pattern for me using the code as pushed here: without
`static { allocServer() ; }` various tests fail in `fuseki1`'s `TS_Fuseki`, but
only ever those that use `resetServer()`. With it
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
That's a weird error, now that I look at it. It suggests that a server
hasn't spun up, but `TestQuery` has
```
// DRY - test protocol?
@BeforeClass public static void beforeClass()
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
RIght, but when I remove `static { allocServer() ; }` the tests for
`jena-fuseki1` no longer pass with
```
org.apache.jena.fuseki.TestQuery Time elapsed: 0.125 sec <<< ERROR!
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
(going off pull/151.diff, which isn't always perfect):
There is still a global server created in fuseki1/ServerTest by:
```
static { allocServer() ; }
```
I don't know why that
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Do you have any changes other than those I picked up with the merge?
If not, the simple way is to work from http_op_3 which I can do - it
includes all your commits on this PR (unlike http_op_2
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I will bang on this more this weekend and try going back down the road you
outlined, @afs . Maybe I missed something in the way I brought your commit
over. If I don't find something, I'll go back
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@afs I'm still getting the errors:
```
Failed tests:
TestRemoteEndpointConnectionWithResultSetTypes>AbstractJenaConnectionTests.connection_statement_query_select_max_rows_01:531
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Hang on, I didn't realize that that was just a simple cherry-pick, doing it
now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@afs Are you saying to take
https://github.com/apache/jena/commit/36b678933ecf2b5553f9345ef84e683cf0f181f0
and add it to this PR, and then we can look at finally merging this into
master? Yes, I can
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
> Let's port the changes here, where the discussion has been.
Branch [http_op_3](https://github.com/afs/jena/tree/http_op_3) is made by:
1. Current Jena master.
1. This PR
1.
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I've opened https://issues.apache.org/jira/browse/JENA-1243 to track the
other clear-up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well.
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
1. Let's port the changes here, where the discussion has been.
2. Absolutely fine with me (to do the other changes elsewhere). I'll make a
ticket for those things now so that we don't lose the
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Can we do the clear-up separately, please? I really want to get he HTTP
changes into the code base as this is in practical terms a blocker on the
release preparation so I'm keen to get what we have into
Github user rvesse commented on the issue:
https://github.com/apache/jena/pull/151
The incorrect/ overridden plug-ins can certainly be removed. They are
probably just cruft from the early days of development.
Historically AspectJ was used in order to inject trace level
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Holy moly, @afs , thanks for the huge assist! I was out yesterday (Yom
Kippur) and am now catching up. I will leave comments specific to the HTTP
question on the new PR, but I will try to speak to the
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
New branch : [http_op_2](https://github.com/afs/jena/tree/http_op_2)
Green line!
This is PR#151, the clean-ups in Jena master and some server control for
jena-jdbc-driver-remote.
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I've cleaned up allocating/deallocing the Fuseki server in case it hides
some timing issues.
Commit AFS-Debug-3 which I have also tried with a multisecond wait after
the server is stopped and
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
This
```
HttpClient hc = HttpOp.createPoolingHttpClient() ;
HttpOp.setDefaultHttpClient(hc);
String queryString = "SELECT * { ?s ?p ?o }" ;
String
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Hm. This is really quite squirrely. If there is something subtle wrong with
`ServerTest::resetServer`, I wonder what I did in my PR to make it show up?
Anyway, I will start tracing it. Thanks for the
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
That stuff in `AbstractRemoteEndpointResultSetTests` is cruft-- client
swapping is taken care of in `TS_JdbcDriverRemote` itself. But it doesn't seem
to have any effect on the mysterious failures. But I
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I have just pushed a branch to https://github.com/afs/jena/tree/http_op
where the tests+setup are extracted into a single class
`AFS_TestRemoteEndpointConnection` and if this is called twice (see
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I found mention of `HttpOp.createCachingHttpClient` in
`AbstractRemoteEndpointResultSetTests` - is that correct?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
It's as though some kind of static state is being impacted. But where?!?!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Yes, I can get it to fail with
```
@RunWith(Suite.class)
@Suite.SuiteClasses({ TestRemoteEndpointConnection.class,
TestRemoteEndpointConnection.class })
public class TestSpecial {}
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I can't get the deadlocking to occur, but by running tests in the order you
give, I can see the three failures in
`TestRemoteEndpointConnectionWithResultSetTypes`. I don't see anything special
about
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Wow, this is getting quite weird. I don't get any of that stuff. I will get
on this and find out why. Deadlock is really unexpected!
---
If your project is set up for it, you can reply to this email
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I pulled this PR into a clean copy of Jena (I did `git pull
https://github.com/apache/jena/ pull/151/head` in to a branch
I get test failures in jena-jdbc-driver-remote: with maven:
```
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Just as an example, I could imagine supporting fluent methods that would
let people use Jena's language selection machinery (i.e. `Lang`) instead of
using HTTP `Accept` directly.
---
If your project
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Like I said, I'm fine with making a new fluent API. Can you describe some
of the things you see as common in particular to a semweb app? `HttpOp` as it
stands doesn't appear to do anything particular to
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
To some extent, yes a fluent API is duplication - as I said I only made a
quick sketch - but the principle of `HttpOp` is to make the use of general HTTP
mechanism provided in forms that wrap up common
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
And as a point of info, I didn't actually end up changing the `HttpOp` API
all that much, just `HttpAuthenticator` => `HttpClient`. The additional
parameters for `HttpContext` turned out to already be
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I'm totally fine with a "high-level" rethink, but at some point, aren't we
just duplicating this:
https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fluent.html
?
---
If your project
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
As for the fix, specifically I went to `HttpOp` and made sure that any HTTP
clients built there get closed properly. HTTP clients that get passed in don't
get closed because that's the job of the client
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Should we take a step back and reconsider whether updating the current API
in HttpOp is the right thing to do.
Maybe we ought to
* introduce a different style more fluid
* reconsider a
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Sorry, @afs , I'm working on this now but not keeping this conversation up
to date. In fact, my most recent comment I now believe to have been wrong. See
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
For information, what's the fix?
I've unmerged this all from my local copy and will start again when we
agree it's ready. If it's easier, maybe drop this and open a new PR when ready?
---
If
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Erm, good/bad news.
1. I can fix the problem @afs found.
2. I don't want to, because I realize now that I did this whole PR wrong.
If you read up this page, you'll see where both
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Got a fix, but I still need to make sure that I didn't break anything else
and that I'm not wasting resources and I'm hitting EOB for the day. I will send
a commit tomorrow morning (EST). Thanks for the
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I've been able to reproduce. Investigating...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
It could be emergent or trigger on my OS. JDBC remote driver uses
QueryEngineHTTP with a retained client and that hits deprecated code.
---
If your project is set up for it, you can reply to this email
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Weird. Maybe something changed out from under me. This is a pretty old PR.
I'll get on it (thanks for the diagnostic info).
---
If your project is set up for it, you can reply to this email and have
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Problem encountered: the JDBC remote driver tests lock up. I have tracked
it down to `TestRemoteEndpointResultsWithGraphUris` run on its own (it locks up
in Eclipse as well) at 113 of the 121 tests.
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Ooh, just noticed that. I will take a look at it today and fix it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
If you prefer, I'll merge it.
(What about the merge conflict?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
It would be great if you could use this stuff in RDFConnector to trial it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Yes, as I understand it, we can merge. I haven't merged it because I try
never to merge my own stuff unless it's pretty trivial.
---
If your project is set up for it, you can reply to this email and
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@rvesse That is _exactly_ what has slowed this down! :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this
Github user rvesse commented on the issue:
https://github.com/apache/jena/pull/151
@ajs6f provided that those examples are available at some point then this
is fine with me. I'm not sure how we could incorporate test cases to this
aspect because you would need an endpoint protected
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Code-wise, not that I know of. But I promised @rvesse examples of doing
form-based authN for query endpoints. If we're okay with those appearing in
docs (instead of the test suite) then I think we could
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Any reason not to apply this? It is in danger of becoming a bit of blocker
on release 3.1.1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Okay. sounds like I owe the following work for this:
- Enriching `HttpOp` to include methods that accept `HttpContext`
- A nice suite of examples that @rvesse approves as a path forward
---
If
Github user rvesse commented on the issue:
https://github.com/apache/jena/pull/151
In a sense I don't have a strong opinion on what the final API should look
like or if a higher level API like `HttpAuthenticator` needs to continue to
exist. Providing there was sufficient examples to
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
But since that construct goes via `HttpClientCOntext`, doesn't that take us
back to methods with `HttpContext` as a parameter? Or am I misunderstanding the
suggestion?
---
If your project is set up
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I came across "preemptive authentication" for HttpClient which apparently
does not a preconfigured `HttpClient`.
[HttpComponents 4.5.x tutorial section,
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@rvesse -- Does @afs 's plan to offer methods that accept
`HttpClientBuilder` meet your concerns about supporting complex authN scenarios?
---
If your project is set up for it, you can reply to this
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Okay, so we should be able to treat 'HttpClientBuilder' as a kind of config
object. Set one up and pass it in the way you would pass in config (including
authN).
---
If your project is set up for it,
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
One question with passing in a builder is the extent to which clients will
be reusable. Perhaps that's where setting a default client or using the
custom-client-per-service feature (available now) would
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@afs Said "(It seems odd that the can't get the setting out of an
HttpClient to feed into a builder.)"
YES. It is _intensely_ annoying. It seems like a massive oversight to me,
but as far as I
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
@rvesse so the proposal is operations that take a `HttpClient` and
operations that take an `HttpClientBuilder` and `HttpAuthenticator`? (It seems
odd that the can't get the setting out of an `HttpClient`
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@afs Yes, the addition of `HttpOp` methods as you describe is what I was
thinking of. Not too much there, but enough to let people move their own
`HttpContext`s around, which would be pretty darn handy
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@paulhoule One difficulty with `HttpRequestInterceptor` is a good thing to
bring up-- in the newer HTTP Commons client APIs, clients are immutable and if
you want to add, e.g., interceptors, you need to
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@paulhoule There is `org.apache.http.HttpRequestInterceptor`, which I think
is what HTTP Commons itself supplies for that kind of purpose. But there are
also a host of parts built into the rest of the
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
@rvesse : Basically, I think we share the same picture of the situation. I
wouldn't claim that this new API presents quite the same "ergonomics" as what
we now have. I think the heart of the matter is
Github user paulhoule commented on the issue:
https://github.com/apache/jena/pull/151
@rvesse can authentication be handled by something like a Decorator?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project
Github user rvesse commented on the issue:
https://github.com/apache/jena/pull/151
It also potentially makes it much harder to define complex authentication
schemes. Four example the current API allows you to define an authenticator
that presents different credentials to different
Github user rvesse commented on the issue:
https://github.com/apache/jena/pull/151
My major concern with this pull request is that it replaces a nicely
encapsulated API with requiring users to directly interact with a lower level
API. It seems like there should be room for compromise
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
So to check my understanding:
For each operation that takes a `HttpClient` there is one that takes a
`HttpClient` and an `HttpContext`.
Not so bad - we don't have the `HttpAutheticator`
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Right, I think we're fine as long as Jena itself is actually the client
(e.g. as part of `SERVICE` action in a query). It was the situation in which
some app is using HTTP via `HttpOp` in which an input
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
The need for closing responses isn't new - all typed input streams should
be closed in the codebase currently because I had to go hunting them all down
with the old `HttpOp` code. So, hopefully, theer
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
Reading through the current HTTP Commons docs leaves me with the impression
that we do not need to close clients after use in `HttpOp`, but that we do need
to see to it that _responses_ are closed.
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
[A useful description of
`HttpContext`.](https://hc.apache.org/httpcomponents-client-ga/tutorial/html/fundamentals.html#d5e223)
---
If your project is set up for it, you can reply to this email and
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I'm imagining a bag of new methods in `HttpOp` similar to what's there now,
with the additional `HttpContext` param, and the methods that are now there
would call through those new methods with `null`
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
What would a sidecar parameter look like in this situation?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
As I understand it (for what that's worth) `HttpContext` is mostly about
the idea of a "conversation", so if you've got some kind of session going on,
or a series of request-response cycles that compose
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
Another thought: Does `HttpContext` with every `HttpClient` usage make an
sense now? We now rely on `HttpClient` a lot more and `HttpContext` seems
mainly for cookies. Maybe, versions of calls with
Github user ajs6f commented on the issue:
https://github.com/apache/jena/pull/151
I'm not totally clear about the style for `CloseableHttpClient`, either.
I'll poke around and see if I can get a better reading on that. And I'll start
jotting up some docs.
---
If your project is set
Github user afs commented on the issue:
https://github.com/apache/jena/pull/151
I haven't got my head around the new API yet - this is just a preliminary
comment.
It is a bit of a change around authentication but I think it is the right
thing to do. It seems to me that the
87 matches
Mail list logo
