Re: [infinispan-dev] Refactoring async operations
On Nov 22, 2012, at 3:03 PM, Dan Berindei dan.berin...@gmail.com wrote:
On Thu, Nov 22, 2012 at 3:31 PM, Mircea Markus mmar...@redhat.com wrote:
On 22 Nov 2012, at 10:16, Dan Berindei wrote:
On Thu, Nov 22, 2012 at 11:53 AM, Galder Zamarreño gal...@redhat.com wrote:
On Nov 21, 2012, at 4:49 PM, Mircea Markus mmar...@redhat.com wrote:
Hi,
Part of fixing ISPN-2435, I need to significantly change
DistributionInterceptor which at the moment is a very complex pice of
code. Building the fix on top of it is extremely difficult and error
prone, so I need to refactor it a bit before moving forward.
One such refactoring is about changing the way the async operations are
handled (e.g. putAsync()). At the moment all the interceptor calls happen
in user's thread, but two remote calls which are invoked with futures and
aggregated:
the L1 invalidation and the actual distribution call. The code for
handling this future aggregation is rather complicated and spreads over
multiple classes (RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor), so the simple alternative solution I have in
mind is to build an asycPut on top of a syncPut and wrap it in a future:
CacheImpl:putAsync(k,v) {
final InvocationContext ic = createInvocatinonContextInCallerThread();
//this is for class loading purpose
return asyncPoolExecutor.submit(new Callable() {
public Object call() {
return put(k,v, ic); //this is the actual sync put
}
}
}
This would significantly simplify several components ( no references to
network/aggregated futures in RpcManager, L1Manager,
ReplicationInterceptor, DistributionInterceptor).
^ At first glance, that's how I'd have implemented this feature, but Manik
went down the route of wrapping in futures only those operations that went
remote.
Maybe he was worried about ctx switch cost? Or maybe about ownership of
locks when these are acquired in a separate thread from the actual caller
thread?
Speaking of locks, does putAsync make sense in a transactional context?
Good point, I don't think async operation should work in the context of
transaction: that would mean having two threads(the async operation thread
and the 'main' thread) working on the same javax.transaction.Transaction
object concurrently which is something not supported by most TM afaik, and
something we don't support internally.
I'm not sure, but I think it is supported now - the only things happening on
a different thread only care about the cache's transaction, and not about the
TM transaction.
^ I think this is important. Even if you call putAsync(), it should participate
in any ongoing transactions without any problems.
@Mircea, what I mean earlier is whether you had prototyped your current
suggestion to have putAsync() submit a callable…etc.
The reason I ask is cos I don't think it should take you very long to prototype
this new way of dealing with async methods, and by running the testsuite you
might encounter other issues, apart from the one implied here wrt transactions.
There may be another backwards compatibility issue here, with listeners that
expect to be called on the caller's thread (e.g. to use the TM transaction
that's stored in a thread-local).
Possible issues:
- caller's class loader - the class loader is aggregated in the
InvocationContext, so as long as we build the class loader in caller's
thread we should be fine
^ To be precise, we don't build a class loaders. I guess you're refering at
building the invocation context.
These days we're more tight wrt the classloader used, avoiding the reliance
on the TCCL, so I think we're in a safer position.
- IsMarshallableInterceptor is used with async marshalling, in order to
notify the user when objects added to the cache are not serializable. With
the approach I suggested, for async calls only (e.g. putAsync) this
notification would not happen in caller's thread, but async on
future.get(). I really don't expect users to rely on this functionality,
but something that would change never the less.
^ I don't think this is crucial. You need to call future.get() to find out
if things worked correctly or not, regardless of cause.
- anything else you can think of?
I know this is a significant change at this stage in the project, so I
really tried to go without it - but that resulted in spaghetti code taking
a lot of time to patch. So instead of spending that time to code a complex
hack I'd rather go for the simple and nice solution and add more unit
tests to prove it works.
^ Have you done some experimenting already?
Cheers,
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
Re: [infinispan-dev] Refactoring async operations
On Nov 21, 2012, at 4:49 PM, Mircea Markus mmar...@redhat.com wrote:
Hi,
Part of fixing ISPN-2435, I need to significantly change
DistributionInterceptor which at the moment is a very complex pice of code.
Building the fix on top of it is extremely difficult and error prone, so I
need to refactor it a bit before moving forward.
One such refactoring is about changing the way the async operations are
handled (e.g. putAsync()). At the moment all the interceptor calls happen in
user's thread, but two remote calls which are invoked with futures and
aggregated:
the L1 invalidation and the actual distribution call. The code for handling
this future aggregation is rather complicated and spreads over multiple
classes (RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor), so the simple alternative solution I have in mind
is to build an asycPut on top of a syncPut and wrap it in a future:
CacheImpl:putAsync(k,v) {
final InvocationContext ic = createInvocatinonContextInCallerThread();
//this is for class loading purpose
return asyncPoolExecutor.submit(new Callable() {
public Object call() {
return put(k,v, ic); //this is the actual sync put
}
}
}
This would significantly simplify several components ( no references to
network/aggregated futures in RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor).
^ At first glance, that's how I'd have implemented this feature, but Manik went
down the route of wrapping in futures only those operations that went remote.
Maybe he was worried about ctx switch cost? Or maybe about ownership of locks
when these are acquired in a separate thread from the actual caller thread?
Possible issues:
- caller's class loader - the class loader is aggregated in the
InvocationContext, so as long as we build the class loader in caller's thread
we should be fine
^ To be precise, we don't build a class loaders. I guess you're refering at
building the invocation context.
These days we're more tight wrt the classloader used, avoiding the reliance on
the TCCL, so I think we're in a safer position.
- IsMarshallableInterceptor is used with async marshalling, in order to
notify the user when objects added to the cache are not serializable. With
the approach I suggested, for async calls only (e.g. putAsync) this
notification would not happen in caller's thread, but async on future.get().
I really don't expect users to rely on this functionality, but something that
would change never the less.
^ I don't think this is crucial. You need to call future.get() to find out if
things worked correctly or not, regardless of cause.
- anything else you can think of?
I know this is a significant change at this stage in the project, so I really
tried to go without it - but that resulted in spaghetti code taking a lot of
time to patch. So instead of spending that time to code a complex hack I'd
rather go for the simple and nice solution and add more unit tests to prove
it works.
^ Have you done some experimenting already?
Cheers,
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Galder Zamarreño
gal...@redhat.com
twitter.com/galderz
Project Lead, Escalante
http://escalante.io
Engineer, Infinispan
http://infinispan.org
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Re: [infinispan-dev] Refactoring async operations
Thanks for the feedback Galder!
On 22 Nov 2012, at 09:53, Galder Zamarreño wrote:
On Nov 21, 2012, at 4:49 PM, Mircea Markus mmar...@redhat.com wrote:
Hi,
Part of fixing ISPN-2435, I need to significantly change
DistributionInterceptor which at the moment is a very complex pice of code.
Building the fix on top of it is extremely difficult and error prone, so I
need to refactor it a bit before moving forward.
One such refactoring is about changing the way the async operations are
handled (e.g. putAsync()). At the moment all the interceptor calls happen in
user's thread, but two remote calls which are invoked with futures and
aggregated:
the L1 invalidation and the actual distribution call. The code for handling
this future aggregation is rather complicated and spreads over multiple
classes (RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor), so the simple alternative solution I have in mind
is to build an asycPut on top of a syncPut and wrap it in a future:
CacheImpl:putAsync(k,v) {
final InvocationContext ic = createInvocatinonContextInCallerThread();
//this is for class loading purpose
return asyncPoolExecutor.submit(new Callable() {
public Object call() {
return put(k,v, ic); //this is the actual sync put
}
}
}
This would significantly simplify several components ( no references to
network/aggregated futures in RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor).
^ At first glance, that's how I'd have implemented this feature, but Manik
went down the route of wrapping in futures only those operations that went
remote.
Maybe he was worried about ctx switch cost? Or maybe about ownership of locks
when these are acquired in a separate thread from the actual caller thread?
Possible issues:
- caller's class loader - the class loader is aggregated in the
InvocationContext, so as long as we build the class loader in caller's
thread we should be fine
^ To be precise, we don't build a class loaders. I guess you're refering at
building the invocation context.
yes, I'm referring to IC which aggregates the ClassLoader
These days we're more tight wrt the classloader used, avoiding the reliance
on the TCCL, so I think we're in a safer position.
- IsMarshallableInterceptor is used with async marshalling, in order to
notify the user when objects added to the cache are not serializable. With
the approach I suggested, for async calls only (e.g. putAsync) this
notification would not happen in caller's thread, but async on future.get().
I really don't expect users to rely on this functionality, but something
that would change never the less.
^ I don't think this is crucial. You need to call future.get() to find out if
things worked correctly or not, regardless of cause.
+1
- anything else you can think of?
I know this is a significant change at this stage in the project, so I
really tried to go without it - but that resulted in spaghetti code taking a
lot of time to patch. So instead of spending that time to code a complex
hack I'd rather go for the simple and nice solution and add more unit tests
to prove it works.
^ Have you done some experimenting already?
Yes, I've pretty much implemented the code without this refactoring. But then
ended up in a loop of fix regressions - introduce new regressions, which was
very hard to break because of the complexity of the code.
Cheers,
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Galder Zamarreño
gal...@redhat.com
twitter.com/galderz
Project Lead, Escalante
http://escalante.io
Engineer, Infinispan
http://infinispan.org
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Re: [infinispan-dev] Refactoring async operations
On 22 Nov 2012, at 10:16, Dan Berindei wrote:
On Thu, Nov 22, 2012 at 11:53 AM, Galder Zamarreño gal...@redhat.com wrote:
On Nov 21, 2012, at 4:49 PM, Mircea Markus mmar...@redhat.com wrote:
Hi,
Part of fixing ISPN-2435, I need to significantly change
DistributionInterceptor which at the moment is a very complex pice of code.
Building the fix on top of it is extremely difficult and error prone, so I
need to refactor it a bit before moving forward.
One such refactoring is about changing the way the async operations are
handled (e.g. putAsync()). At the moment all the interceptor calls happen
in user's thread, but two remote calls which are invoked with futures and
aggregated:
the L1 invalidation and the actual distribution call. The code for handling
this future aggregation is rather complicated and spreads over multiple
classes (RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor), so the simple alternative solution I have in mind
is to build an asycPut on top of a syncPut and wrap it in a future:
CacheImpl:putAsync(k,v) {
final InvocationContext ic = createInvocatinonContextInCallerThread();
//this is for class loading purpose
return asyncPoolExecutor.submit(new Callable() {
public Object call() {
return put(k,v, ic); //this is the actual sync put
}
}
}
This would significantly simplify several components ( no references to
network/aggregated futures in RpcManager, L1Manager,
ReplicationInterceptor, DistributionInterceptor).
^ At first glance, that's how I'd have implemented this feature, but Manik
went down the route of wrapping in futures only those operations that went
remote.
Maybe he was worried about ctx switch cost? Or maybe about ownership of locks
when these are acquired in a separate thread from the actual caller thread?
Speaking of locks, does putAsync make sense in a transactional context?
Good point, I don't think async operation should work in the context of
transaction: that would mean having two threads(the async operation thread and
the 'main' thread) working on the same javax.transaction.Transaction object
concurrently which is something not supported by most TM afaik, and something
we don't support internally.
There may be another backwards compatibility issue here, with listeners that
expect to be called on the caller's thread (e.g. to use the TM transaction
that's stored in a thread-local).
Possible issues:
- caller's class loader - the class loader is aggregated in the
InvocationContext, so as long as we build the class loader in caller's
thread we should be fine
^ To be precise, we don't build a class loaders. I guess you're refering at
building the invocation context.
These days we're more tight wrt the classloader used, avoiding the reliance
on the TCCL, so I think we're in a safer position.
- IsMarshallableInterceptor is used with async marshalling, in order to
notify the user when objects added to the cache are not serializable. With
the approach I suggested, for async calls only (e.g. putAsync) this
notification would not happen in caller's thread, but async on
future.get(). I really don't expect users to rely on this functionality,
but something that would change never the less.
^ I don't think this is crucial. You need to call future.get() to find out if
things worked correctly or not, regardless of cause.
- anything else you can think of?
I know this is a significant change at this stage in the project, so I
really tried to go without it - but that resulted in spaghetti code taking
a lot of time to patch. So instead of spending that time to code a complex
hack I'd rather go for the simple and nice solution and add more unit tests
to prove it works.
^ Have you done some experimenting already?
Cheers,
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Galder Zamarreño
gal...@redhat.com
twitter.com/galderz
Project Lead, Escalante
http://escalante.io
Engineer, Infinispan
http://infinispan.org
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Re: [infinispan-dev] Refactoring async operations
On Thu, Nov 22, 2012 at 3:31 PM, Mircea Markus mmar...@redhat.com wrote:
On 22 Nov 2012, at 10:16, Dan Berindei wrote:
On Thu, Nov 22, 2012 at 11:53 AM, Galder Zamarreño gal...@redhat.comwrote:
On Nov 21, 2012, at 4:49 PM, Mircea Markus mmar...@redhat.com wrote:
Hi,
Part of fixing ISPN-2435, I need to significantly change
DistributionInterceptor which at the moment is a very complex pice of code.
Building the fix on top of it is extremely difficult and error prone, so I
need to refactor it a bit before moving forward.
One such refactoring is about changing the way the async operations are
handled (e.g. putAsync()). At the moment all the interceptor calls happen
in user's thread, but two remote calls which are invoked with futures and
aggregated:
the L1 invalidation and the actual distribution call. The code for
handling this future aggregation is rather complicated and spreads over
multiple classes (RpcManager, L1Manager, ReplicationInterceptor,
DistributionInterceptor), so the simple alternative solution I have in mind
is to build an asycPut on top of a syncPut and wrap it in a future:
CacheImpl:putAsync(k,v) {
final InvocationContext ic =
createInvocatinonContextInCallerThread(); //this is for class loading
purpose
return asyncPoolExecutor.submit(new Callable() {
public Object call() {
return put(k,v, ic); //this is the actual sync put
}
}
}
This would significantly simplify several components ( no references to
network/aggregated futures in RpcManager, L1Manager,
ReplicationInterceptor, DistributionInterceptor).
^ At first glance, that's how I'd have implemented this feature, but
Manik went down the route of wrapping in futures only those operations that
went remote.
Maybe he was worried about ctx switch cost? Or maybe about ownership of
locks when these are acquired in a separate thread from the actual caller
thread?
Speaking of locks, does putAsync make sense in a transactional context?
Good point, I don't think async operation should work in the context of
transaction: that would mean having two threads(the async operation thread
and the 'main' thread) working on the same javax.transaction.Transaction
object concurrently which is something not supported by most TM afaik, and
something we don't support internally.
I'm not sure, but I think it is supported now - the only things happening
on a different thread only care about the cache's transaction, and not
about the TM transaction.
There may be another backwards compatibility issue here, with listeners
that expect to be called on the caller's thread (e.g. to use the TM
transaction that's stored in a thread-local).
Possible issues:
- caller's class loader - the class loader is aggregated in the
InvocationContext, so as long as we build the class loader in caller's
thread we should be fine
^ To be precise, we don't build a class loaders. I guess you're refering
at building the invocation context.
These days we're more tight wrt the classloader used, avoiding the
reliance on the TCCL, so I think we're in a safer position.
- IsMarshallableInterceptor is used with async marshalling, in order to
notify the user when objects added to the cache are not serializable. With
the approach I suggested, for async calls only (e.g. putAsync) this
notification would not happen in caller's thread, but async on
future.get(). I really don't expect users to rely on this functionality,
but something that would change never the less.
^ I don't think this is crucial. You need to call future.get() to find
out if things worked correctly or not, regardless of cause.
- anything else you can think of?
I know this is a significant change at this stage in the project, so I
really tried to go without it - but that resulted in spaghetti code taking
a lot of time to patch. So instead of spending that time to code a complex
hack I'd rather go for the simple and nice solution and add more unit tests
to prove it works.
^ Have you done some experimenting already?
Cheers,
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
___
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
