Explicit effect types

[igor-vovk]

This addresses #2379.
Overall I deleted more code than I've added which is a good sign.

• AkkaHttpClient and PekkoHttpClient were wrapping a lot of Future's under the hood – this is now removed and looks much cleaner.
• JavaClient now uses Scala Promise/Future mechanism that doesn't require any 3rd-party libraries and that was created to replace callback mechanisms, I think it is better this way
• SttpRequestHttpClient was a wrapper around Future monad's Sttp implementation – now allows any monads that are supported by Sttp, feels like a proper way to use it.
• Testkit now explicitly define that Future effect is used, which was used under the hood before as well, just not explicitly shown.
• After the change Executor's became empty since all handling is now performed on the Client's level. I left them just to show that they are now obsolete, will be happy to remove them as well.
• After removing Executors, I tend to think that effect-zio, effect-zio-1, effect-cats, effect-cats-2, effect-monix, effect-scalaz modules can be also removed as a future improvement (not sure about that, this may require cats dependency to reuse Functor that is now defined manually in elastic4s-core and that those modules implement).
• ZIO tests are not compiling now because I don't know which ZIO client to use. But it doesn't make sense for me to have ZIO implementation without ZIO-based http clients, not sure what to do here.

I would add that the way the callback mechanism was defined and used looked like a big hack for me, that was maybe historically implemented because library started with only JavaClient? I didn't check this assumption, but overall, everything that Executor's and Client's were doing, is mapping from K[_] monad implemented by 3rd-party clients, through (Either[Throwable, A]) => Unit callback, to F[_] monad used on the top-level. So there was this clunky 3-step transformation that hidden a lot of stuff.

[igor-vovk]

To fix cats-effect tests, this PR #3118 needs to be merged first and then http4s client needs to be used which supports attributary effects

[igor-vovk]

Tests are now fixed. I've added sttp ZIO backends to test ZIO implementations. We can add some kind of a table in docs to express which backend can be used with the monad of choice:

• Future: JavaClient, SttpClient with Future backend
• ZIO 1/2: SttpClient with respective ZIO backend https://sttp.softwaremill.com/en/stable/backends/zio.html
• cats-effect: SttpClient with cats-effect backend https://sttp.softwaremill.com/en/stable/backends/catseffect.html, Http4sClient (from another PR)
• Scalaz: SttpClient with scalaz backend https://sttp.softwaremill.com/en/stable/backends/scalaz.html

[igor-vovk]

After this change, sttpFutureBackend can be marked as Provided in build.sbt, since the library now supports multiple Sttp backends, there is no reason anymore to depend on a future backend (maybe we can create a separate issue for this)

[lenguyenthanh]

After this change, sttpFutureBackend can be marked as Provided in build.sbt, since the library now supports multiple Sttp backends, there is no reason anymore to depend on a future backend (maybe we can create a separate issue for this)

I think this is a big change, We only can release this in a major version bump (hopefully 9). So, I guess We should rip the band-aid by removing Executor and clean up things like this in subsequent prs. I could help if needed.

[igor-vovk]

@lenguyenthanh yes backwards compatibility went sideways with this PR. Still, I think it's a change long needed. In the last commit custom executors were replaced with a single generic Executor, that kinda does the same. Still Executors are used in tests, where they accomplish a role of overriding ElasticRequest and adding some headers in the request lifecycle. Maybe someone uses them this way? 🤔

[lenguyenthanh]

Still Executors are used in tests, where they accomplish a role of overriding ElasticRequest and adding some headers in the request lifecycle. Maybe someone uses them this way? 🤔

I think We can achieve the same functionality with HttpClient[F] => HttpClient[F] 🤔. So, We can mention that what used to do with Executor now can achieve with HttpClient.

[igor-vovk]

Yeah, I can think this can be achieved with ElasticRequest => ElasticRequest method even, which will be even simpler

[Philippus]

Indeed this will not be a change that can be merged without a major version bump. I can make an RC release or something like that if that is handy for testing.

[lenguyenthanh]

Indeed this will not be a change that can be merged without a major version bump. I can make an RC release or something like that if that is handy for testing.

An RC is great, thanks! I guess We can have this and #3118 and maybe another pr to removing Executor (as it is obsolete) for the new major release.

[igor-vovk]

I think we will also need to change the docs in that case, and can add removal of Sttp Future backend as a default dependency for the Sttp client. I've been also thinking how to get rid of elastic4s-effect-* modules, since they now only define a Functor, but it doesn't look like they can be easily removed.

[lenguyenthanh]

I think we will also need to change the docs in that case, and can add removal of Sttp Future backend as a default dependency for the Sttp client.

yes, We definitely need docs and a proper change log for this.

I've been also thinking how to get rid of elastic4s-effect-* modules, since they now only define a Functor, but it doesn't look like they can be easily removed.

I think the decision We need to make here is do We want to pull cats dependencies to core module for cats.Functor or not. If We don't, We need to keep these modules.

I prefer to user cats.Functor directly as I believe it's quite ubiquitous in Scala world at this point and it keeps binary compatibility for last several years.

[igor-vovk]

@lenguyenthanh the problem with cats.Functor is that it will be hard to make it compatible with cats-effect-1. I doubt if people still use it tho. As well as scalaz, monix, zio-1. Maybe those modules should be removed, I don't know?

[lenguyenthanh]

IMHO, I think We can and We should remove all elastic4s-effect-* module as you mentioned earlier. With version 9, it'll be a big breaking change but We can keep maintain version 8 for like another 6 months (back port all bug and security fixes). I can help on that if needed as well.

But this is ultimately @Philippus's decision.

[lenguyenthanh]

this looks great, thanks @igor-vovk for your work!

I think We should create a branch (something like v8.0.0) to merge all @igor-vovk's prs and then publish a RC, what do you think @Philippus ?

[lenguyenthanh]

@Philippus We would very appreciate, if you can comment on how to proceed this PR (and other related pre as well) 🙏

I think this is a great improvement as it's simplify the code base a lot, remove many concepts and that leads to less allocations, better ergonomic code. It is especially helpful for cats users as We can use async IO from the top to bottom without any intermediate layers.

[lenguyenthanh]

Hi @Philippus sorry to ping you again and again. But I really love to try this other PRs that @igor-vovk's spend time to work on.

Could you have some word about these prs?

It's fine if you don't think this is the way forward but please tell us so.

[Philippus]

I will try to look into it soon, have to find some time.

[lenguyenthanh]

thanks @Philippus, much appreciated 🙏