On 17/12/2014 09:58, Keiichi Fujino wrote: > 2014-12-17 7:52 GMT+09:00 Mark Thomas <ma...@apache.org>:
<snip/> >> In summary, patches [7] and [8] look to be the ones that need the most >> careful consideration followed by [2]. >> >> Thoughts? >> >> > [1][2][3][4][5][6][9][A] > +1 back-port. > There is no objection here. > > [7][8] > I have few comment. > > [A] I think that the process of updating SSO entry needs to be notify > cluster. I'll take a look at this. I didn't see any issues in my simple testing but I may well have missed this scenario. > [B] The mapSendOptions and terminateOnStartFailure attributes do not need > to be configurable? Fair point. I'll add that. > [C] Is deregister(String ssoId, Session session) method required? > I do not know where it will be called. I'll look into it. > [D] If my understanding is correct, > Session objects are associated with SingleSignOnEntry of primary node > and SingleSignOn is registered as a SessionListener. > I think this is no problem. > However, when the primary node goes down, because SingleSignOnEntry of > non-primary node is > not associated with the session object, SingleSignOn can not receive a > SessionEvent. > As a result, SingleSignOnEntry does not delete because the deregister > method is no longer called. > This might be a potential memory leak. My testing definitely didn't cover this. I'll add some logging and extend my tests. > At least, the new node that was promoted to the primary node needｓ to > associate the session object again. > For example, the association of session might be able to in > objectMadePrimary method. > However, because there is a concern of [E], is not called objectMadePrimary > method. > > [E] Although I think this is another potential problem, > ReplicatedMap sends a COPY message to non-primary node. > Node that received the COPY message does not save the entry as a > backup(MapEntry.backup=false). > As a result, MapEntry.isPrimary() always returns true. > I do not know yet whether this is a problem, > but, at least, processing of entry relocate in memberDisappeared seem to > not work properly. Since every MapEntry should be replicated in full to every node (I went with ReplicatedMap rather than LazyReplicatedMap) I don't think this is an issue but I'll look in to this as well. > Node that received the COPY message may have to save the entry as a backup > or copy(newly added). Thanks for such a thorough review. While it raises a number of issues it actually makes me more convinced that this was the right way to fix this as ReplicatedMap has thought through all of these issues already rather than trying to re-invent the wheel for SSO. Thanks again, Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
