Having trouble logging in, so I will post my comments here. Sorry if this isn't according to standards, but please advise otherwise.
Space between Parameterized types https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-35e57ba52a1021a9998b91e63d9ce02eR59 new HashMap<String, String>(), this is in a lot of places. Probably good to do a clean-up when possible. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-3fdab6dbe84793d47aa0da8f75794c8cR55 Map<String, String> properties https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-c03a3ff6884dfea893f8ccc4cab2d1b0R44 new HashMap<String, String>() https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-a882881b342f8208577c330128eb16fdR39 Time to address this now? https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-d235710e38d43916e0915eb6c838ebe0R177 Gossipper -> Gossiper. I recommend doing a global search/replace for this one to pick up other classes and fields, too. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-74123e0e74657b59d77ec906fcc0468fR51 You can do some method extract in this class. For instance, lines 73-81 can be extracted to a new method that returns a GossipCore. Or further down one that returns a UdpActiveGossipMessage. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-74123e0e74657b59d77ec906fcc0468fR73 Maybe rename these args to "self" and "otherMember"? https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-74123e0e74657b59d77ec906fcc0468fR67 This should become a property. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-9d369418fc6dc1e95f8b3f8a35d554a7R41 Lines 67-110 have some indentation inconsistencies. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-9d369418fc6dc1e95f8b3f8a35d554a7R67 Shurdown -> Shutdown https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-b93576f92c0cc381f8b00acf684e5039R82 Maybe late to the party, but is this the way we are going to write tests? I'm a big fan of given-when-then so that the test cases are more readable, easier to modify and more succinct. Right now, I have to read the entire method to understand what is under test. https://github.com/apache/incubator-gossip/compare/master...edwardcapriolo:GOSSIP-39?expand=1#diff-cf515c8f73af947a0a6d307043ff8dc6R25 On Tue, Jan 24, 2017 at 1:55 PM Dorian Ellerbe <[email protected]> wrote: > Same here. > > On Mon, Jan 23, 2017 at 11:40 PM chandresh pancholi < > [email protected]> wrote: > > I will review it by today. > > On Tue, Jan 24, 2017 at 4:03 AM, Edward Capriolo <[email protected]> > wrote: > > > Hey all, > > > > One of the big ticket items I want for the next release is the ability to > > control how gossip peers chose each other. For example It might be > possibly > > to gossip more aggressively inside a LAN and less aggressively outside. > > This is an attempt at this: > > > > https://github.com/apache/incubator-gossip/compare/ > > master...edwardcapriolo:GOSSIP-39?expand=1 > > > > I am not 100% ready to merge but getting close. So a preliminary review > > would help. > > > > Thanks, > > Edward > > > > > > -- > Chandresh Pancholi > Senior Software Engineer > Flipkart.com > Email-id:[email protected] > Contact:08951803660 > >
