1 2 Previous Next 18 Replies Latest reply on Feb 9, 2012 6:01 PM by clebert.suconic

    Notes on trunk

    borges

      Hi,

       

      I am pushing loads of changes into trunk, and I will take the opportunity to make some notes:

       

      • there seems to be a bug in surefire which was triggered by our fork=always timeout=600 combo. As the IO confirmation hang that lead us to introduce the fork=always hasn't occurred since my 'attempt' of fixing it (HORNETQ-820 @ SVN r11963), I reverted to fork=once.
      • I changed the internal usage of URL in XmlDeployer to URI. URL.equals and URL.hashCode make internet access making them very problematic to use as keys in HashMap's.
      • I changed may things in the synchronization of large-messages between live/backup, the new code addresses some problems and is  simpler.

       

      As always there are loads of small correctness fixes to the code.

       

      I will be spending my next few days revising the synchronization of the Paging system, which seems to be messy ('messy' being an understatement given the failing tests).

       

      [...]

       

      yes, this kind of message is better suited to a mailing-list than to a forum, but since we don;t have a mailing-list, the forum gets it.

        • 1. Re: Notes on trunk
          clebert.suconic

          By synchronization you mean replication synchronization, right?

           

          It's your implementation.. change it as you wish.

           

           

           

          yes, this kind of message is better suited to a mailing-list than to a forum, but since we don;t have a mailing-list, the forum gets it.

          We're open source... as long as you don't talk about company matters (customers, or internal issues.. this is a good forum).

          • 2. Re: Notes on trunk
            clebert.suconic

            As always there are loads of small correctness fixes to the code.

             

            Francisco, I'm going to ask you a favour..

             

             

            As we talked before.. it's going to be extremely hard to merge changes from Branch_2_2 to Trunk because of these changes.

             

            IMO you should have focused on replication and making it stable only.. We had this talk before.

             

             

            For instance... you changed the ordering on ClusterTopologyChangeMessage, and we also had changes on Branch_2_2... I will have to manually inspect every class on the system to make a valid merge.

             

            So, I'm starting the merge now.. I would ask you to at least until I finish this merge to not do any cosmetic changes. Only semantic changes at least until I finish this merge please.

            • 3. Re: Notes on trunk
              clebert.suconic

              I will be looking into doing a two-way merge...  get some of the javadocs changes you made towards AS7 / EAP also.

              • 4. Re: Notes on trunk
                borges

                Clebert Suconic wrote:

                 

                As always there are loads of small correctness fixes to the code.

                 

                Francisco, I'm going to ask you a favour..

                 

                 

                As we talked before.. it's going to be extremely hard to merge changes from Branch_2_2 to Trunk because of these changes.

                 

                IMO you should have focused on replication and making it stable only.. We had this talk before.

                 

                For instance... you changed the ordering on ClusterTopologyChangeMessage, and we also had changes on Branch_2_2... I will have to manually inspect every class on the system to make a valid merge.

                 

                I can't be sure replication works unless all the other tests pass as well (i.e. trunk having clean builds), say, it just doesn't work when there are 150 failing tests (now down to 40), or when the build never completes because of hanging tests.

                 

                Regarding ClusterTopologyChangeMessage, its changes are:

                -- deleting trailing-whitespace;

                -- addition of equals/hashCode at the bottom (as overriding of .equals/.hashCode was inconsistent).

                 

                How any of that creates merging difficulties is beyond me.

                 

                Kind regards,

                Francisco

                • 5. Re: Notes on trunk
                  clebert.suconic

                  Was there a reason (beyond cosmetics) to change FileDeployementManager to use URI instead of URI?

                   

                   

                   

                  Anyway, I'm fine with the change.. it's just that I'm dealing with the merge and I'm seeing these changes.. but I'm not sure if it was changed on trunk or 2.2.

                  • 6. Re: Notes on trunk
                    clebert.suconic

                    @Francisco: I will use this thread to address a few of my questions. Please refer the orignal body when you answer them...

                     

                     

                    Another question:

                     

                    You made getAddress a synchronized method on MessageImpl.

                     

                    That method should never be accessed by more than a single thread (the address should at least by static on this context.. except to when we initialize it which is single threaded). Was there any reason you made it synchronized?

                    • 7. Re: Notes on trunk
                      borges

                      Clebert Suconic wrote:

                       

                      Was there a reason (beyond cosmetics) to change FileDeployementManager to use URI instead of URI?

                       

                       

                       

                      Anyway, I'm fine with the change.. it's just that I'm dealing with the merge and I'm seeing these changes.. but I'm not sure if it was changed on trunk or 2.2.

                      Assuming you meant changes between URL --> URI.

                       

                      As I mentioned in commit message, because URL.equals/hashCode will make internet access.

                       

                      A longer explantion goes:

                      URL.equals() is a blocking operation, and its return value will depend on the network status at the moment you call it (it performs name resolution). URL.hashCode is also blocking as it needs to resolve "all the URL components relevant for URL comparison". Given this unreliability, it is problematic to add URL instances to HashMap's or HashSet's, which is what was happening at FileDeployementManager.

                       

                      See http://docs.oracle.com/javase/6/docs/api/java/net/URL.html#equals%28java.lang.Object%29

                      • 8. Re: Notes on trunk
                        clebert.suconic

                        ahhhh... ok.. now I understand it... it makes sense.

                         

                        But we are only using this for File Access. it wouldn't go over the internet.. what would make it a minor issue, right?

                         

                         

                        What about the Message.getAddress(); Did you have any reason to make it synchronized? you shouldn't have a reason ... that's what I want to understand now.

                        • 9. Re: Notes on trunk
                          borges

                          Clebert Suconic wrote:

                           

                           

                          You made getAddress a synchronized method on MessageImpl.

                           

                          That method should never be accessed by more than a single thread (the address should at least by static on this context.. except to when we initialize it which is single threaded). Was there any reason you made it synchronized?

                          Because, again as mentioned in the commit message, the address synchronization was inconsistent. That is inconsistent between getAddress() (which was unsynchronized) and setAddress() (which was synchronized).

                           

                          If you are sure that none of the 23 references to getAddress() will never be in a race against (the 19 references to) setAddress(). Feel free to remove the synchronization, but please do document why we need a synchronized setAddress() and why leaving a getAddress() unsynchronized is ok. Otherwise that will just keep confusing anyone reading the code.

                           

                          I don't doubt that it is safe to leave getAddress() unsynchronized as you say, but if it is, it must be documented in the code in order to avoid leaving people scratching their heads when they read it.

                          • 10. Re: Notes on trunk
                            borges

                            Clebert Suconic wrote:

                             

                            But we are only using this for File Access. it wouldn't go over the internet.. what would make it a minor issue, right?

                             

                            We had the same situation with XmlDeployer, just changing all of them is clearer and allows us to change the Deployer (internal) interface to use URIs instead of URLs.

                            • 11. Re: Notes on trunk
                              clebert.suconic

                              Francisco Borges wrote:

                               

                              Clebert Suconic wrote:

                               

                               

                              You made getAddress a synchronized method on MessageImpl.

                               

                              That method should never be accessed by more than a single thread (the address should at least by static on this context.. except to when we initialize it which is single threaded). Was there any reason you made it synchronized?

                              Because, again as mentioned in the commit message, the address synchronization was inconsistent. That is inconsistent between getAddress() (which was unsynchronized) and setAddress() (which was synchronized).

                               

                              If you are sure that none of the 23 references to getAddress() will never be in a race against (the 19 references to) setAddress(). Feel free to remove the synchronization, but please do document why we need a synchronized setAddress() and why leaving a getAddress() unsynchronized is ok. Otherwise that will just keep confusing anyone reading the code.

                               

                              I don't doubt that it is safe to leave getAddress() unsynchronized as you say, but if it is, it must be documented in the code in order to avoid leaving people scratching their heads when they read it.

                               

                               

                              I didn't write that code.. it's part of the original design by the author of the class at the time (Tim Fox).

                               

                              The reason it's synchronized it's because of the encoding of the message. It's protecting the buffer (look at bufferValid), not the address itself.

                               

                              From what i see on the code also, this method is only called from a single thread.. but I'm not brave enough to remove it... so I will keep it.

                               

                               

                              However adding synchronized on getAddress will add something to the runtime.. and I don't think we want to do that.. getAddress is immutable after the message is routed or received.. so.. I will remove / document that.

                              • 12. Re: Notes on trunk
                                clebert.suconic

                                Just a status update on my progress....

                                 

                                 

                                It's taking longer than I expected... I'm comparing source by source... I will give an update tomorrow as if I will be able to finish early next week.

                                • 13. Re: Notes on trunk
                                  clebert.suconic

                                  Can't we remove ReplicatedLargeMessage as an interface?

                                   

                                  It seems that the InSyncLargeMessage could just implement the LargeServerMessage?

                                  • 14. Re: Notes on trunk
                                    borges

                                    Clebert Suconic wrote:

                                     

                                    Can't we remove ReplicatedLargeMessage as an interface?

                                     

                                    It seems that the InSyncLargeMessage could just implement the LargeServerMessage?

                                     

                                    I added that interface because it is a sub-set of the LargeServerMessage that contains all that is used during Replication, making code inspection for completeness a lot easier. That was the only reason. It made the code easier to understand for me.

                                     

                                    I prefer to keep it, but if there is a preference to remove it, it is no big deal.

                                    1 2 Previous Next