[ http://issues.apache.org/jira/browse/DERBY-989?page=comments#action_12419148 ]
Bryan Pendleton commented on DERBY-989: --------------------------------------- Thanks for posting the patch. I think it is good work, and I am persuaded that you have the right theory and have identified the right problem to fix. Your explanation of how this change fixes the problem seems good to me, and your testing results certainly show that the patch is effective in removing the problem. I'm not very familiar with this code, so these comments are mostly for my own education. 1) I don't really see why unsubscribe needs to block if it discovers that the service record being unsubscribed is currently performing work. Is this a necessary element of the fix, or just a side effect of the implementation? 2) I think that it is dangerous for unsubscribe to ignore interrupts while waiting for the service record to complete its work. I think that if unsubscribe gets an interrupt at this time, it should abandon the wait and return, or it should throw an exception. 3) Introducing the "client number" concept into the service record seems awkward, as it doesn't seem like a natural part of the service record implementation. Also, comparing client numbers as a way to answer the question "is this service record already unsubscribed" seems somewhat convoluted. Here are two alternate ideas that occurred to me: - perhaps we could refer to the new Service Record field as a "unique id", and have an "equals" method to compare two ServiceRecord objects, so that we don't so directly couple the Client Number concept from Basic Daemon into the ServiceRecord class. - perhaps there should be an "subscribed" boolean flag in Service Record, with methods "isSubscribed" and "setSubscribed" so that when Basic Daemon wants to ask if a ServiceRecord is already unsubscribed, it could do so directly rather then by fetching the client number and testing it against -1 and so forth. 4) It seems a little odd that BasicDaemon sometimes synchronizes on the "this" object, and sometimes depends on the fact that "subscription" is a Vector and thus is inherently synchronized. I'm not saying that anything is necessarily wrong here, it was just a red flag to me that there were two levels of synchronization being used in the BasicDaemon methods. > unit/daemonService.unit fails intermittently: 'ran out of time' > --------------------------------------------------------------- > > Key: DERBY-989 > URL: http://issues.apache.org/jira/browse/DERBY-989 > Project: Derby > Type: Test > Components: Regression Test Failure > Versions: 10.2.0.0 > Environment: OS: Solaris 10 3/05 s10_74L2a X86 - SunOS 5.10 Generic, JVM: > Sun Microsystems Inc. 1.5.0_04 > OS: Solaris 9 9/04 s9s_u7wos_09 SPARC - SunOS 5.9 Generic_118558-11, JVM: Sun > Microsystems Inc. 1.5.0_03 > OS: Red Hat Enterprise Linux AS release 3 (Taroon Update 4) - Linux > 2.4.21-27.ELsmp #1 SMP Wed Dec 1 21:50:31 EST 2004 GNU/Linux, JVM: Sun > Microsystems Inc. 1.5.0_03 > Reporter: Ole Solberg > Assignee: Knut Anders Hatlen > Priority: Minor > Attachments: 989-411220-derbyall_derbyall_unit_unit_derby.log, > derby-989-timebomb.diff, derby-989-timebomb.stat, derby-989-v1.diff, > derby-989-v1.stat > > "Signature": > ********* Diff file unit/unit/daemonService.diff > *** Start: daemonService jdk1.5.0_04 unit:unit 2006-02-14 20:46:42 *** > 2 del > < -- Unit Test T_DaemonService finished > 2 add > > ran out of time > > Shutting down due to unit test failure. > > Exit due to time bomb > Test Failed. > *** End: daemonService jdk1.5.0_04 unit:unit 2006-02-14 21:47:13 *** > http://www.multinet.no/~solberg/public/Apache/Derby/Limited/testSummary-377800.html > [SunOS-5.10 i86pc-i386] -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
