> What's the benefit of this catch-up game?
Not tainting our SPI with RestClient :)
Yoann Rodière
Hibernate NoORM Team
yo...@hibernate.org
On 2 June 2017 at 14:59, Gunnar Morling <gun...@hibernate.org> wrote:
> Sure, as I said, an implementor of that SPI will very likely deal with
> the HTTP client. But our SPI isn't tainted by that.
>
> I also don't see much value in re-defining all the options from
> RestClientBuilder. ElasticsearchHttpClientConfigurer resembles
> HttpClientConfigCallback. Would you also re-define
> RequestConfigCallback? What if new options get added to
> RestClientBuilder? What's the benefit of this catch-up game?
>
>
>
>
> 2017-06-02 14:47 GMT+02:00 Yoann Rodiere <yo...@hibernate.org>:
> >> There's no exposure of HTTP Client in this SPI. Yes, if people need to
> >> customize the HTTP client to be used by the returned RestClient
> >> instance, they'll naturally depend on that. But this SPI isn't tied to
> >> such detail of how RestClient works - if ES folks decided to use
> >> OkHttp instead, our SPI contract won't be affected (of course user's
> >> implementations will need to change if they were customizing the HTTP
> >> client before).
> >
> > I would feel dishonest arguing this to users... Frankly, there's little
> > point in returning a custom RestClient without customizing stuff related
> to
> > Apache HTTP Client. See RestClientBuilder: apart from methods tied to
> Apache
> > HTTP Client, and from options already provided by Hibernate Search, you
> only
> > have access to these:
> >
> > org.elasticsearch.client.RestClientBuilder.setDefaultHeaders(Header[])
> > org.elasticsearch.client.RestClientBuilder.setFailureListener(
> FailureListener)
> > org.elasticsearch.client.RestClientBuilder.setPathPrefix(String)
> >
> > ... and that's all.
> >
> > So yes, this SPI doesn't have a direct dependency to Apache HTTP Client,
> but
> > any practical use of it will have one. At the end of the day, that's what
> > really matters, right?
> >
> > Yoann Rodière
> > Hibernate NoORM Team
> > yo...@hibernate.org
> >
> > On 2 June 2017 at 14:25, Gunnar Morling <gun...@hibernate.org> wrote:
> >>
> >> 2017-06-02 12:49 GMT+02:00 Yoann Rodiere <yo...@hibernate.org>:
> >> >> There's an important difference: one exposes Apache HTTP client in
> >> >> HSEARCH's SPI, whereas the other just requires usage of Apache HTTP
> >> >> client within one specific implementation. For users it doesn't
> change
> >> >> much, but the latter is cleaner from HSEARCH's perspective.
> >> >
> >> > RestClient is not an interface, it's an implementation. There's no
> >> > interface. So yes, we would just be exposing Apache HTTP Client on top
> >> > of
> >> > RestClient.
> >> >
> >>
> >> Here's what I'd have done:
> >>
> >> package org.hibernate.search.elasticsearch.client.spi;
> >>
> >> public interface RestClientFactory {
> >> RestClient buildRestClient(SomeContext ctx);
> >> }
> >>
> >> There's no exposure of HTTP Client in this SPI. Yes, if people need to
> >> customize the HTTP client to be used by the returned RestClient
> >> instance, they'll naturally depend on that. But this SPI isn't tied to
> >> such detail of how RestClient works - if ES folks decided to use
> >> OkHttp instead, our SPI contract won't be affected (of course user's
> >> implementations will need to change if they were customizing the HTTP
> >> client before).
> >>
> >> >> I'm not quite following on that. If people are in control of the
> >> >> RestClient entirely, they can do whatever they want?
> >> >
> >> > As mentioned above, exposing RestClient is even worse than just
> exposing
> >> > the
> >> > Apache HTTP client.
> >>
> >> I don't think it's worse, it's better actually. It just exposes our
> >> direct dependency in the SPI and not any of its details.
> >>
> >> > So I was suggesting to use a proper abstraction, namely our own
> >> > interface,
> >> > org.hibernate.search.elasticsearch.client.impl.ElasticsearchClient.
> >> >
> >> > I suppose your remark concerned this paragraph:
> >> >
> >> >> On top of that, this solution would not allow multiple third-party
> >> >> customizations to work well together (for instance AWS authentication
> >> >> provided by us or a third party + some performance tweaking by the
> >> >> user)...
> >> >> which is something the SPI we're planning in the PR could allow.
> >> >
> >> > If we did allow users to re-implement ElasticsearchClient, yes, one
> >> > implementor would be able to do whatever he wants... if he
> re-implements
> >> > it.
> >> > He wouldn't be able to re-use other extensions.
> >> > Take for example the AWS authentication. You're suggestion that we
> >> > provide
> >> > an alternative client allowing to connect to AWS. Fine, we do that,
> and
> >> > users can use it. But what if a users wants AWS authentication and
> say,
> >> > configure a proxy? Then he can't reuse our AWS client, since this
> client
> >> > is
> >> > just an implementation of ElasticsearchClient, and we don't want to
> >> > expose
> >> > anything related to Apache HTTP Client. So he must implement AWS
> >> > authentication. Just to configure a proxy.
> >> >
> >> > The thing is, there are tons of things a user may want to do with
> Apache
> >> > HTTP Client, and we can't possibly provide access to each and every
> >> > option
> >> > through an abstraction layer.
> >>
> >> Right, hence I wouldn't bother to do that in the first place. Just let
> >> users customize how RestClient is instantiated. Let's them do all they
> >> want. We can provide examples which show how to do the AWS signing
> >> etc.
> >>
> >> > So at some point, if we want to allow
> >> > configuration (and in the case of an HTTP client, I'm afraid we have
> >> > to),
> >> > we'll have to expose internals. We just have to make sure this is done
> >> > in a
> >> > controlled way (expose as little as possible).
> >> >
> >> >
> >> >
> >> > Yoann Rodière
> >> > Hibernate NoORM Team
> >> > yo...@hibernate.org
> >> >
> >> > On 2 June 2017 at 12:26, Gunnar Morling <gun...@hibernate.org> wrote:
> >> >>
> >> >> 2017-06-02 11:58 GMT+02:00 Yoann Rodiere <yo...@hibernate.org>:
> >> >> >> Did you instead consider to just let users provide their custom
> >> >> >> instance of org.elasticsearch.client.RestClient? It's still
> leaking
> >> >> >> an
> >> >> >> implementation detail of Hibernate Search, but at least it's one
> >> >> >> indirection less.
> >> >> >
> >> >> >
> >> >> > The only way to add AWS authentication to the RestClient is to use
> >> >> > Apache
> >> >> > HTTP Client classes, so this solution would still bind users to the
> >> >> > implementation details. We'd just shove it under the carpet :)
> >> >>
> >> >> There's an important difference: one exposes Apache HTTP client in
> >> >> HSEARCH's SPI, whereas the other just requires usage of Apache HTTP
> >> >> client within one specific implementation. For users it doesn't
> change
> >> >> much, but the latter is cleaner from HSEARCH's perspective.
> >> >> >
> >> >> > Also, I'd argue this would be an even bigger implementation leak:
> at
> >> >> > least
> >> >> > with the current solution with can switch to an alternative
> >> >> > Elasticsearch
> >> >> > client, as long as we still use Apache HTTP Client. If we expose
> >> >> > RestClient,
> >> >> > we're stuck with it and whatever underlying technologies it uses.
> >> >> >
> >> >> > On the other hand, we could ask them to re-implement
> >> >> > org.hibernate.search.elasticsearch.client.impl.
> ElasticsearchClient,
> >> >> > which is
> >> >> > our own façade over the Elasticsearch client. But I find it a bit
> >> >> > much
> >> >> > just
> >> >> > to tweak some settings or to add a new authentication scheme...
> >> >> > On top of that, this solution would not allow multiple third-party
> >> >> > customizations to work well together (for instance AWS
> authentication
> >> >> > provided by us or a third party + some performance tweaking by the
> >> >> > user)...
> >> >> > which is something the SPI we're planning in the PR could allow.
> >> >>
> >> >> I'm not quite following on that. If people are in control of the
> >> >> RestClient entirely, they can do whatever they want?
> >> >>
> >> >> >
> >> >> > Yoann Rodière
> >> >> > Hibernate NoORM Team
> >> >> > yo...@hibernate.org
> >> >> >
> >> >> > On 2 June 2017 at 10:30, Sanne Grinovero <sa...@hibernate.org>
> wrote:
> >> >> >>
> >> >> >> On Fri, 2 Jun 2017, 08:56 Gunnar Morling, <gun...@hibernate.org>
> >> >> >> wrote:
> >> >> >>
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > I find the exposure of an implementation detail (usage of Apache
> >> >> >> > HTTP
> >> >> >> > client) of the Elasticsearch client a bit problematic. If they
> >> >> >> > change
> >> >> >> > this to another HTTP client, our SPI would break.
> >> >> >> >
> >> >> >>
> >> >> >> Yes the very point of exposing that detail is the reason for this
> >> >> >> thread.
> >> >> >>
> >> >> >> Still, our SPI being guaranteed only for a minor, that gives a lot
> >> >> >> of
> >> >> >> flexibility?
> >> >> >>
> >> >> >> The Elasticsearch client exposing this itself, I don't expect them
> >> >> >> to
> >> >> >> switch implementation in a micro release to make some bugfix. If
> >> >> >> they
> >> >> >> change it in a major or even minor version, we're ok to not
> support
> >> >> >> that
> >> >> >> version until our next minor.
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> > Did you instead consider to just let users provide their custom
> >> >> >> > instance of org.elasticsearch.client.RestClient? It's still
> >> >> >> > leaking
> >> >> >> > an
> >> >> >> > implementation detail of Hibernate Search, but at least it's one
> >> >> >> > indirection less.
> >> >> >> >
> >> >> >> > People wishing to have a custom RestClient would have to
> implement
> >> >> >> > a
> >> >> >> > few more bits themselves (the logic from
> >> >> >> > DefaultElasticsearchClientFactory#customizeHttpClientConfig()),
> >> >> >> > but
> >> >> >> > I'd find that acceptable for the sake of a less detail-exposing
> >> >> >> > SPI,
> >> >> >> > plus it grants more flexibility in terms of configuring the
> >> >> >> > RestClient.
> >> >> >> >
> >> >> >>
> >> >> >>
> >> >> >> N.B. The client factory already is a Service so any advanced user
> >> >> >> already
> >> >> >> can override it.
> >> >> >>
> >> >> >> We want to make it easier for cloud users. Focusing on AWS now as
> >> >> >> we've
> >> >> >> had
> >> >> >> user requests for this - not least our own CI - but I'd expect
> other
> >> >> >> clouds
> >> >> >> to have similar features (today or tomorrow). I just don't expect
> >> >> >> other
> >> >> >> use
> >> >> >> cases to need this so we might provide them all eventually, but at
> >> >> >> this
> >> >> >> point my goal is to leave an appealing SPI for contributors to
> join
> >> >> >> on
> >> >> >> that.
> >> >> >>
> >> >> >> With that I mean:
> >> >> >> 1# this might evolve but we need something simple to use for
> people
> >> >> >> to
> >> >> >> not
> >> >> >> get stuck.
> >> >> >> 2# I expect integrator implementors to contribute them back
> >> >> >> 3# People won't have this low level dependency in their projects
> for
> >> >> >> long
> >> >> >>
> >> >> >> Having them re-implement the client wouldn't encourage this ;)
> >> >> >>
> >> >> >> Thanks!
> >> >> >> Sanne
> >> >> >>
> >> >> >>
> >> >> >> > --Gunnar
> >> >> >> >
> >> >> >> >
> >> >> >> > 2017-06-01 19:11 GMT+02:00 Sanne Grinovero <sa...@hibernate.org
> >:
> >> >> >> > > Yoann has been working on allowing Hibernate Search users to
> use
> >> >> >> > > Elasticsearch on AWS.
> >> >> >> > >
> >> >> >> > > Specifically on AWS the Elasticsearch security can be
> configured
> >> >> >> > > to
> >> >> >> > > use application identities, which implies the requests need to
> >> >> >> > > be
> >> >> >> > > signed.
> >> >> >> > >
> >> >> >> > > A good background read can be found here [1].
> >> >> >> > >
> >> >> >> > > We planned to allow people to use this but were not planning
> to
> >> >> >> > > include AWS specific libraries as dependencies - but since
> Yoann
> >> >> >> > > implemented an actual AWS signer in the tests I suppose it
> would
> >> >> >> > > be
> >> >> >> > > selfish to not ship it..
> >> >> >> > >
> >> >> >> > > Please see the API proposal on github (with the PR):
> >> >> >> > > - https://github.com/hibernate/hibernate-search/pull/1426
> >> >> >> > >
> >> >> >> > > Thanks,
> >> >> >> > > Sanne
> >> >> >> > >
> >> >> >> > > [1] -
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >> > https://aws.amazon.com/blogs/security/how-to-control-
> access-to-your-amazon-elasticsearch-service-domain/
> >> >> >> > > _______________________________________________
> >> >> >> > > hibernate-dev mailing list
> >> >> >> > > hibernate-dev@lists.jboss.org
> >> >> >> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >> >> >> >
> >> >> >> _______________________________________________
> >> >> >> hibernate-dev mailing list
> >> >> >> hibernate-dev@lists.jboss.org
> >> >> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
