2017-06-02 12:29 GMT+02:00 Sanne Grinovero <sa...@hibernate.org>: > On 2 June 2017 at 10:58, Yoann Rodiere <yo...@hibernate.org> wrote: >>> 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 :) >> >> 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. > > +1 > > I think you nailed it design wise. > > Gunnar, I have the impression we're simply not aligned on the definition of > SPI. > > How to do custom request signing on a *specific platform* (AWS) with a > *specific client* (this ES client using the Apache HTTP client) is > going to need a specific adaptor; the good news is that this is going > to be isolated so the main user application code - entities and > business logic - won't be affected.
Sure, I don't see where either proposal is different in that regard? > That's SPI by the book and we're not giving guarantees of backwards > compatibility beyond a minor. > > We know the Elasticsearch driver is using the Apache HTTP client and > this won't change overnight; if they happen to change it, we'll change > in a new minor release and people will have to adapt. Understood that this is your rule. I don't find it a good one, though. IMO, minor releases shouldn't break APIs nor SPIs for that matter. YMMV. > > Too bad but it's practical. Let me also point out a drawback of your > alternative solution: the user's code for those we re-implement the > driver would not break but only if they fail to upgrade the driver, > which is possibly another pandora box. I don't follow. WDYM by "re-implementing the driver"? All I suggested is letting people instantiate their own RestClient. > > If you feel strongly about it I'm ok to promote the > `DefaultElasticsearchClientFactory` to SPI to give some more options > but I see no much value in that. Send a PR if you want it ;) I don't feel strongly about it - you asked for feedback on the proposal, so that's what I gave. I'd personally avoid to tie our SPIs to implementation specifics of dependencies. Nothing more, nothing less. > > Thanks, > Sanne > >> >> 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 _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
