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
