Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

evaluate the possibility of Future API surface #533

Closed
baywet opened this issue Oct 8, 2020 · 8 comments · Fixed by #610
Closed

evaluate the possibility of Future API surface #533

baywet opened this issue Oct 8, 2020 · 8 comments · Fixed by #610
Assignees
Labels
ADO to GitHub automation label Issue caused by core project dependency modules or library enhancement under-investigation
Milestone

Comments

@baywet
Copy link
Member

baywet commented Oct 8, 2020

Context: java's equivalent to Promises/Tasks are Futures. Adding an API surface that returns Futures (and potentially removing the callback one??) would enable our customers to write modern Java in their applications

I suppose so but my callback wrapper is pretty straightforward; it wasn't a huge deal writing it.

object ICallbackWrapper extends StrictLogging {

  private def cbF[T](): (Future[T], ICallback[T]) = {
    val p = Promise[T]()
    val cb = new ICallback[T] {
      override def success(result: T): Unit = {
        val _ = p.trySuccess(result)
      }

      override def failure(ex: ClientException): Unit = {
        val _ = p.tryFailure(ex)
      }
    }

    (p.future, cb)
  }

  def toFut[T](fun: ICallback[T] => Unit)(
      implicit
      ec: ExecutionContext,
  ): Future[T] = {
    val (f, cb) = cbF[T]()

    Limiter.apiFut {
      () =>
        fun(cb)
        f
    }
  }

  def toFut[V, T](v: V, fun: (V, ICallback[T]) => Unit)(
      implicit
      ec: ExecutionContext,
  ): Future[T] = {
    val (f, cb) = cbF[T]()

    Limiter.apiFut {
      () =>
        fun(v, cb)
        f
    }
  }

  def uploadFut[V, T](v: V, len: Long, fun: (V, ICallback[T]) => Unit)(
      implicit
      ec: ExecutionContext,
  ): Future[T] = {
    val (f, cb) = cbF[T]()

    Limiter.attachFut(
      () => {
        fun(v, cb)
        f
      },
      len,
    )
  }
}

By far the biggest issue with using graph are the very restrictive rate limits... I shimmed a "rate limiter" into the above wrapper so I'm able to limit both number of simultaneous connections as well as ensure uploads don't go over the 15mb/30 sec limit. It took a lot of trial and error getting the futures passed just right so I didn't go over the max 4 connection cap, but it seems to be working well in practice.

Originally posted by @lespea in https://github.com/microsoftgraph/msgraph-sdk-java/issue_comments/705202102
AB#6315

@ghost ghost added the ToTriage label Oct 8, 2020
@baywet baywet self-assigned this Oct 8, 2020
@ghost ghost removed the ToTriage label Oct 8, 2020
@baywet baywet added this to the 3.0.0 milestone Oct 8, 2020
@baywet baywet added enhancement ADO to GitHub automation label Issue caused by core project dependency modules or library labels Oct 8, 2020
@baywet
Copy link
Member Author

baywet commented Nov 17, 2020

@baywet
Copy link
Member Author

baywet commented Nov 26, 2020

alternatively the azure identity is using Mono, which requires API level 26 but apparently has a better design

@baywet
Copy link
Member Author

baywet commented Dec 14, 2020

alternatively, android had asyncTask

@baywet
Copy link
Member Author

baywet commented Dec 18, 2020

update: whatever solution we come up with should work with API level 21.

@baywet
Copy link
Member Author

baywet commented Dec 29, 2020

additionally, most http client libraries seem to be providing a native support for Futures (not okhttpclient though)
https://www.mocklab.io/blog/which-java-http-client-should-i-use-in-2020/

@baywet
Copy link
Member Author

baywet commented Jan 8, 2021

alternatively, CompletableFuture API level 24 (java allows for chaining the futures (comparatively to "plain" futures)

@baywet
Copy link
Member Author

baywet commented Jan 8, 2021

Recap of the solution and work to be done:
We'll provide a Future API surface which will replace the callback API surface, as having both seems unnecessary and providing a future API surface seems superior to a callback one to comply with our compatibility requirements.
This API surface needs to be wired all the way down to the okhttp call, using FutureTasks, and wrapping the enqueue call. (execute needs to be replaced where used)
Any custom Callback and executor definition needs to be stripped out.

We might need to rely on guava's ListenableFutures to chain the work as basic Futures don't seem to provide that kind of support and as CompletableFuture is only available on API level 24.

Lastly, we'll keep providing a synchronous API to align with Azure SDKs

Note: azure provides completable futures and monos in their SDK, but they have separate SDKs for android and java

@baywet
Copy link
Member Author

baywet commented Jan 13, 2021

Update, after investigating and looking in depth at telemetry, we'll switch to CompletableFutures and raise the API level to 26 see the original comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADO to GitHub automation label Issue caused by core project dependency modules or library enhancement under-investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant