You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gossip.apache.org by Edward Capriolo <ed...@gmail.com> on 2017/01/23 22:33:11 UTC
Review of Gossip-39
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
Re: Review of Gossip-39
Posted by Dorian Ellerbe <do...@gmail.com>.
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 <do...@gmail.com>
wrote:
> Same here.
>
> On Mon, Jan 23, 2017 at 11:40 PM chandresh pancholi <
> chandreshpancholi007@gmail.com> wrote:
>
> I will review it by today.
>
> On Tue, Jan 24, 2017 at 4:03 AM, Edward Capriolo <ed...@gmail.com>
> 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:chandresh.pancholi@flipkart.com
> Contact:08951803660
>
>
Re: Review of Gossip-39
Posted by Dorian Ellerbe <do...@gmail.com>.
Same here.
On Mon, Jan 23, 2017 at 11:40 PM chandresh pancholi <
chandreshpancholi007@gmail.com> wrote:
> I will review it by today.
>
> On Tue, Jan 24, 2017 at 4:03 AM, Edward Capriolo <ed...@gmail.com>
> 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:chandresh.pancholi@flipkart.com
> Contact:08951803660
>
Re: Review of Gossip-39
Posted by chandresh pancholi <ch...@gmail.com>.
I will review it by today.
On Tue, Jan 24, 2017 at 4:03 AM, Edward Capriolo <ed...@gmail.com>
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:chandresh.pancholi@flipkart.com
Contact:08951803660