jsr107 / jsr107spec

JSR107 Cache Specification
Apache License 2.0
413 stars 164 forks source link

async API #307

Closed RuedigerMoeller closed 9 years ago

RuedigerMoeller commented 9 years ago

There should be an asynchronous variant of most important operations such as get(), getAndPut(), putIFAbsent(), .. using a functional style callback or a Future alike return object. Considering most implementations will be implemented in a distributed fashion, an enforced synchronous API creates a direct connection of throughput and network latency, so an application deployed in the cloud will perform like 100 times worse compared to a deployment running a 10GB lowlat network LAN.

e.g. see https://github.com/RuedigerMoeller/advcalendar2014/blob/master/src/main/java/keyvalue/KVServer.java for an example of an completely async kv-store example. Throughput of this example is completely independent of network latency.

brianoliver commented 9 years ago

About JCache spec being local-only. If you take a look at EntryProcessor, for example, it is not Serializable because apparently JCache is local-only and cannot have distributed semantics on the API. Take a look at the thread #309 https://github.com/jsr107/jsr107spec/issues/309 . It does not seem fair to pick and choose on a case by case basis when the spec should be local-only and when it should be distributed. Your assumption here is completely incorrect.

JCache is definitely not “local-only”. Almost every open-source and commercial vendor that provides distributed caching support for EntryProcessors does / can do so without requiring the implementations to be serializable.

The specification and historic discussions on this topic is very clear on this. It’s simply not acceptable to force all vendors and applications to implement Serializable, especially when almost none require it, mostly due to the poor performance applications experience when using it!

Should Apache Ignite require it, then it should say so (as mentioned in the specification). Apache Ignite requirements for Serialization should however not force everyone to use it, especially when it’s so bad!

Asynchronous operations are only useful when synchronous operations take a long time. For caches this is only true for distributed operations. For example, there is a reason that java.util.HashMap does not have Async API - it just does not make sense there. Again, your assumption here is completely incorrect.

Imagine a “local-cache” as you say, that has a CacheWrite configured and the said CacheWriter is for a device that has extremely long latencies, where “extremely long” is simply an unacceptable time according to the SLA of an application. eg: it might be milliseconds instead of microseconds, or microseconds instead of nanoseconds. It may not even mean going across the wire!

This is a clear example of when “asynchronous” may be beneficial but it’s not a “distributed-cache”. I only say it “may” be beneficial as some vendors provide custom CacheWrite wrappers here to support asynchronous writing and thus there’s no reason for an asynchronous Cache API.

ie: The motivation for asynchronous operations has nothing to do with “local” v’s “distributed”. That may be the case for Apache Ignite, but don’t assume that is the same for every provider and every application.

As far as Apache Ignite way of handling asynchronous operations, yes, you are right about the internal context. Whenever you create an asynchronous instance of Cache API in Apache Ignite, it does store the future for the previous invocation inside the internal ThreadLocal state. However, I am not sure why it would be a big deal. When it comes to APIs, elegant design and usability should come first in my view. Elegant Design is simply in the eye of the beholder. There is simply no qualifiable “measure” of elegance. What is elegant to one person may not necessarily be elegant to another (hence the response from Chris)

What I personally find alarming is that when using an Apache Ignite asynchronous cache, an application essentially becomes single threaded! The way it’s designed (unless you’re also using a ThreadLocal) is that no two threads can make calls onto the same asynchronous cache as it can only handle one Future at a time! Perhaps the implementation doesn’t work like that, in which case it’s doing a lot more work.

We’ve also discussed many times the significant problems with using Java Future for collecting asynchronous results, which is why we’ve avoided using it. The pitfalls are very significant when APIs do this (a colleague and I presented on this fact at Java One a few years back to a packed audience).

https://www.youtube.com/watch?v=L-rKLSdPEMs&list=PLxqhEJ4CA3JuxH_XbEpeA0IqkSAf4VQZX&index=5 https://www.youtube.com/watch?v=L-rKLSdPEMs&list=PLxqhEJ4CA3JuxH_XbEpeA0IqkSAf4VQZX&index=5

Interestingly most of the new asynchronous APIs I’ve seen lately (eg: reactive) also avoid it! We should do so as well!

The paramount requirements for API design in a specification are correctness, then implementability and testability. Everything else comes later. Usability is way down the list of priorities as a misleading API or one that allows developers to create applications with undesirable / unexpected behavior is completely unacceptable. Hence why we’ve asked so many questions about the proposed async (and transaction) APIs thus far. They look nice but so far all fail to answer the most basic correctness / semantics questions, meaning they are untestable.

Future.cancel() should return true or false. If a future cannot be canceled, then its cancel method should return false. Providers should be able to implement this. I’m not sure you’ve read the specification of the Future.cancel method.

http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#cancel(boolean) http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#cancel(boolean)

It clearly states "Attempts to cancel execution of this task”

So in the case of “put(…).cancel()” it must attempt to cancel the put! By using Future, you’re saying all operations are cancelable!

To me this makes absolutely no sense as all in a Cache API! This is not what we’re being asked to do here. Furthermore taking the view that;

  1. Should providers not be able or not want to implement it then it’s safe to return false is completely unacceptable. That concept breaks application portability as one application that may actually be required to cancel and operation won’t be able to switch to another provider that decides (on their own whim) not to support it.
  2. We can assuming Providers can implement something.

This is not how specifications are developed.

There are all kinds of reasons some providers can / can’t implement certain features, say with without considerable effort. A simple example is the requirement to introduce a change in a wire protocol to support a feature. For most vendors there’s often a combinatorial explosion of testing when something like this happens (say for backwards and forwards compatibility testing), in which case doing so needs to happen for a very good reason. Adding the ability to “cancel” a put is certainly not one of those in my view.

Reply to this email directly or view it on GitHub https://github.com/jsr107/jsr107spec/issues/307#issuecomment-75909731.

gregrluck commented 9 years ago

Moved to #312