[PIO-182] Add async methods to LEventStore #482

takezoe · 2018-10-15T13:12:15Z

This pull request contains following updates:

Add async (non-blocking) methods to LEventStore and LJavaEventStore
Add Scaladoc about scala.concurrent.ExecutionContext.Implicits.global which is used in blocking methods

See also: https://lists.apache.org/thread.html/f14e4f8f29410e4585b3d8e9f646b88293a605f4716d3c4d60771854@%3Cuser.predictionio.apache.org%3E

takezoe · 2018-10-16T16:13:07Z

@dszeto @shimamoto Could you take a look?

Chris who raised this issue wants to do adding the asynchrnous processing capability to other storages and engine server following this pull request.

shimamoto · 2018-10-17T09:03:06Z

data/src/main/scala/org/apache/predictionio/data/store/LEventStore.scala

@@ -89,6 +103,57 @@ object LEventStore {
      timeout)


I think it is preferable to call the findByEntityAsync method from this method.

shimamoto · 2018-10-17T09:05:12Z

data/src/main/scala/org/apache/predictionio/data/store/LEventStore.scala

@@ -142,4 +209,56 @@ object LEventStore {
      limit = limit), timeout)


same as above

Fixed as well.

takezoe · 2018-10-17T13:50:01Z

data/src/main/scala/org/apache/predictionio/data/store/LEventStore.scala

+  * export JAVA_OPTS="$JAVA_OPTS \
+  *   -Dscala.concurrent.context.numThreads=1000 \
+  *   -Dscala.concurrent.context.maxThreads=1000"
+  * </pre>


@dszeto Is this comment understandable? Since I'm not confident that it explains correctly and clearly, I'd like to request your confirmation.

I think we can add a:
Since using scala.concurrent.ExecutionContext.Implicits.global is not a good implementation, we only import it in the blocking method if the user is calling it.

@Wei-1 Thanks for your feedback! I added line comment on importing as below:

// Import here to ensure ExecutionContext.Implicits.global is used only this method import scala.concurrent.ExecutionContext.Implicits.global

btw, only in this method?

Oops, fixed. thanks!

LGTM. We can link to https://docs.scala-lang.org/overviews/core/futures.html#the-global-execution-context for a detail explanation if you'd like to.

Wei-1 · 2018-10-19T06:20:42Z

data/src/main/scala/org/apache/predictionio/data/store/LEventStore.scala

@@ -72,9 +84,61 @@ object LEventStore {
    latest: Boolean = true,
    timeout: Duration = defaultTimeout): Iterator[Event] = {

+    import scala.concurrent.ExecutionContext.Implicits.global


Sorry to ask, but I will like to clarify.
The reason why we import this line in the object instead of putting it on top is because of:
importing global will introduce a lazy ExecutionContext class
Since it will only be created when it is called, we import it in each thread separately so we won't be using one single ExecutionContext.
Is my interpretation correct?

Since using ExecutionContext.Implicits.global is bad habit in Scala basically, I wanted to make sure to use it only in these methods.

And your interpretation is correct. Even if we call it twice, it returns a same instance.

longliveenduro · 2018-10-24T09:59:07Z

Any chance that this gets merged in develop soon? Would need to base my async work on that

takezoe · 2018-10-24T15:53:04Z

@longliveenduro How do you feel about Scaladoc of LEventStore.scala in this pull request? Is explanation is enough and clear? I'm waiting for someone's review for that.

longliveenduro · 2018-10-25T08:41:36Z

@takezoe The scaladoc is clear I think.

But after some days of letting this settle and also reading this excellent blog: https://www.beyondthelines.net/computing/scala-future-and-execution-context/

I am not sure if it is really a good idea to use the "standard" Scala Execution Context for the "old blocking" code. Another idea would we (like proposed in the Blog) use a separate Thread pool, make the defaults the same as now (or maybe raise the allowed thread count to bypass/hack the problem from now on a bit for usages of the old code) and make the size of the thread pool configurable like the standard scala thread pool.

For example this blog suggests that a FixedThreadPool for blocking purposes might be a good idea:
https://www.cakesolutions.net/teamblogs/demystifying-the-blocking-construct-in-scala-futures

takezoe · 2018-10-25T16:15:22Z

@longliveenduro Even if we use thread for asynchronous processing, we should use separated thread pools for CPU bound processing and I/O bound processing. The thread pool for CPU bound processing should have threads close to the number of CPUs. One for I/O bound processing should have threads significantly larger than the number of CPUs.

Anyway, I think that allowing to give an execution context from user side is good strategy because it makes possible to control by users.

longliveenduro · 2018-10-26T10:16:09Z

@takezoe yes exactly, seperate Threadpools for CPU and I/O bound blocking processing looks like a very good idea for a transition phase. Also looking at the Blogs they suggest a fixed Threadpool instead of a ForkJoin Threadpool (which the "standard" Scala thread pool is) for blocking I/O code.

Anyhow, looking at your PR, you are importing the standard Scala execution context in the methods that still use blocking via Await.result. What about using a separate Threadpool there whose size is either configurable or you pass it as a parameter. Passing as a parameter though is breaking the current contract and is a little bit strange since the method does not return an async instance (e.g. Future) which I would expect from a scala method which needs an ExecutionContext param.

Very interested what you think!

takezoe · 2018-10-26T10:26:31Z

@longliveenduro Using the default execution context in this pull request is just for backward compatibility with the current blocking methods. Making it configurable might be better, but we can increase the number of threads by giving a VM option. I think it's a sufficient workaround.

dszeto · 2018-10-26T18:42:32Z

How about we push for deprecating blocking methods? We can update docs to instruct developers to start using non-blocking methods instead.

takezoe · 2018-10-28T12:06:37Z

I'm not sure if deprecating current blocking methods in the next version is good. I would like to see following work by @longliveenduro before making a decision.

takezoe · 2018-10-29T12:15:58Z

Anyway, I'm going to merge. Thanks for all reviews!

takezoe and others added 2 commits October 15, 2018 20:50

Add Future methods and Scaladoc about ExecutionContext

5164e3f

Update Scaladoc

ec0b6e4

takezoe force-pushed the leventstore-async branch from 54af5f4 to 77e2f36 Compare October 16, 2018 06:48

Add async methods to LJavaEventStore

54269c6

takezoe force-pushed the leventstore-async branch from 77e2f36 to 54269c6 Compare October 16, 2018 07:23

shimamoto reviewed Oct 17, 2018

View reviewed changes

Delegate blocking methods to async methods

a9dc66d

takezoe commented Oct 17, 2018

View reviewed changes

Wei-1 reviewed Oct 19, 2018

View reviewed changes

Add comment about internal ExecutionContext.Implicits.global importing

fae05f4

takezoe force-pushed the leventstore-async branch from 7e6f437 to fae05f4 Compare October 25, 2018 00:23

Add link to the Scala documentation

709f4d3

takezoe merged commit a479fe3 into apache:develop Oct 29, 2018

takezoe deleted the leventstore-async branch October 29, 2018 14:15

		@@ -142,4 +209,56 @@ object LEventStore {
		limit = limit), timeout)

[PIO-182] Add async methods to LEventStore #482

[PIO-182] Add async methods to LEventStore #482

Uh oh!

Conversation

takezoe commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

takezoe commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longliveenduro commented Oct 24, 2018

Uh oh!

takezoe commented Oct 24, 2018

Uh oh!

longliveenduro commented Oct 25, 2018

Uh oh!

takezoe commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longliveenduro commented Oct 26, 2018

Uh oh!

takezoe commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dszeto commented Oct 26, 2018

Uh oh!

takezoe commented Oct 28, 2018

Uh oh!

takezoe commented Oct 29, 2018

Uh oh!

Uh oh!

takezoe commented Oct 15, 2018 •

edited

Loading

takezoe commented Oct 25, 2018 •

edited

Loading

takezoe commented Oct 26, 2018 •

edited

Loading