<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Thanks for the feedback Galder!<div><br><div><div>On 22 Nov 2012, at 09:53, Galder Zamarreņo wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>On Nov 21, 2012, at 4:49 PM, Mircea Markus <<a href="mailto:mmarkus@redhat.com">mmarkus@redhat.com</a>> wrote:<br><br><blockquote type="cite">Hi,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">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.<br></blockquote><blockquote type="cite">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:<br></blockquote><blockquote type="cite">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:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">CacheImpl:putAsync(k,v) {<br></blockquote><blockquote type="cite"> final InvocationContext ic = createInvocatinonContextInCallerThread(); //this is for class loading purpose<br></blockquote><blockquote type="cite"> return asyncPoolExecutor.submit(new Callable() {<br></blockquote><blockquote type="cite"> public Object call() {<br></blockquote><blockquote type="cite"> return put(k,v, ic); //this is the actual sync put<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> } <br></blockquote><blockquote type="cite">} <br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This would significantly simplify several components ( no references to network/aggregated futures in RpcManager, L1Manager, ReplicationInterceptor, DistributionInterceptor).<br></blockquote><br>^ 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. <br><br>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?</div></blockquote><blockquote type="cite"><div><br><blockquote type="cite">Possible issues:<br></blockquote><blockquote type="cite">- 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<br></blockquote><br>^ To be precise, we don't build a class loaders. I guess you're refering at building the invocation context.<br></div></blockquote>yes, I'm referring to IC which aggregates the ClassLoader<br><blockquote type="cite"><div><br>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.<br></div></blockquote><br><blockquote type="cite"><div><br><blockquote type="cite">- 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. <br></blockquote><br>^ 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.<br></div></blockquote>+1<br><blockquote type="cite"><div><br><blockquote type="cite">- anything else you can think of?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">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.<br></blockquote><br>^ Have you done some experimenting already?<br></div></blockquote><div>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. </div><br><blockquote type="cite"><div><br>Cheers,<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite">Cheers,<br></blockquote><blockquote type="cite">-- <br></blockquote><blockquote type="cite">Mircea Markus<br></blockquote><blockquote type="cite">Infinispan lead (<a href="http://www.infinispan.org">www.infinispan.org</a>)<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">_______________________________________________<br></blockquote><blockquote type="cite">infinispan-dev mailing list<br></blockquote><blockquote type="cite"><a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br></blockquote><blockquote type="cite"><a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br></blockquote><br><br>--<br>Galder Zamarreņo<br><a href="mailto:galder@redhat.com">galder@redhat.com</a><br>twitter.com/galderz<br><br>Project Lead, Escalante<br>http://escalante.io<br><br>Engineer, Infinispan<br>http://infinispan.org<br><br><br>_______________________________________________<br>infinispan-dev mailing list<br>infinispan-dev@lists.jboss.org<br>https://lists.jboss.org/mailman/listinfo/infinispan-dev<br></div></blockquote></div><br><div apple-content-edited="true">
<span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Cheers,</div><div>-- <br>Mircea Markus</div><div>Infinispan lead (<a href="http://www.infinispan.org">www.infinispan.org</a>)</div><div><br></div></div></span></div></span></div></span></div></span><br class="Apple-interchange-newline"></span><br class="Apple-interchange-newline">
</div>
<br></div></body></html>