ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From uday kale <udaygk...@gmail.com>
Subject Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented
Date Fri, 21 Dec 2018 15:30:31 GMT
Hi,

Regarding the name, it's not just about being a good name. I would point to
the official AWS documentation [1], [2]. The current ELB does not stand for
just the Classic Load balancer. Based on this documentation, the naming is
good. I agree with your suggestion about extending
TcpDiscoveryApplicationElbIpFinder from the TcpDiscoveryClassicElbIpFinder.
I can make the required changes.

[1]
https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/introduction.html

[2]
https://docs.aws.amazon.com/elasticloadbalancing/latest/application/introduction.html


Regards,
Uday




On Thu, Dec 20, 2018 at 12:09 AM Stanislav Lukyanov <stanlukyanov@gmail.com>
wrote:

> Hi,
>
> Regarding the name - a not-so-good name isn’t always a sufficient
> justification for renaming.
> Public products such as Ignite have to also take into account convenience
> for existing users.
> Even in 3.0 when we technically can break compatibility I would avoid
> breaking it just to
> rename “ELB” to “Classic ELB”.
>
> Also, I believe the official names Amazon uses for these technologies are
> ELB and ALB,
> not “classic ELB” and “application ELB”.
>
> Regarding the code - `extends TcpDiscoveryClassicElbIpFinder` doesn’t
> really solve it.
> For example, now you have an unused loadBalancerName property in the
> TcpDiscoveryApplicationElbIpFinder.
>
> I guess, to move this forward, let’s just add a new class,
> TcpDiscoveryAlbIpFinder.
> It doesn’t have to extend the existing TcpDiscoveryElbIpFinder – maybe we
> could merge them, but let’s not be
> stuck on this any longer.
> Also let’s not change the existing TcpDiscoveryElbIpFinder – no need to
> rename it or deprecate it. We can thing of the
> two IpFinders as of just independent features.
>
> Thanks,
> Stan
>
> From: uday kale
> Sent: 9 октября 2018 г. 19:34
> To: dev@ignite.apache.org
> Subject: Re: Volunteer needed: AWS Elastic Load Balancer IP
> Findersimplemented
>
> Hi Stanislav,
>
> I have removed extended the TcpDiscoveryApplicationElbIpFinder from
> TcpDiscoveryClassicElbIpFinder to limi the code duplication. Please share
> your feedback.
>
> Thanks,
> Uday
>
> On Wed, Oct 3, 2018 at 1:12 AM Dmitriy Pavlov <dpavlov.spb@gmail.com>
> wrote:
>
> > Hi Uday,
> >
> > We should keep classes name as is within all minor releases (2.x). We can
> > schedule rename only to 3.0. We need to keep both classes and methods
> > naming as is to provide compilation guarantee. If someone used existing
> > TcpDiscoveryElbIpFinder from 2.6 release than his or her code should
> > compile and run well.
> >
> > I've updated checklist, thank you for pointing to this.
> >
> > So I suggest keeping existing naming of TcpDiscoveryElbIpFinder as it is
> > part of API.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > вт, 2 окт. 2018 г. в 21:04, uday kale <udaygkale@gmail.com>:
> >
> > > Thanks for the review Stan.
> > > I renamed the existing class TcpDiscoveryElbIpFinder since the name is
> > not
> > > clear regarding the actual load balancer being used. The names
> > > TcpDiscoveryClassicElbIpFinder
> > > and TcpDiscoveryApplicationElbIpFinder provide a clear distinction.
> > > I deprecated the existing class because of review checklist [1] point
> 1.a
> > > under checklist. It requires methods to be deprecated instead of being
> > > renamed. I assumed this will extend to classes too.
> > > Regarding the your suggestion for having the tests I agree. It will be
> > > helpful to know whether TC can accept properties while initiating a
> run.
> > >
> > > [1]
> https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist
> > >
> > > Uday
> > >
> > > On Tue, Oct 2, 2018 at 9:58 PM Stanislav Lukyanov <
> > stanlukyanov@gmail.com>
> > > wrote:
> > >
> > > > Also, it makes me nervous that we’re lacking any sort of functional
> > tests
> > > > for AWS-based IP finders (same for GCE, actually).
> > > > I understand that it’s hard to include tests like that in regular TC
> > runs
> > > > because we’d have to include some sort of credentials.
> > > > But let’s think of some sort of a middle ground.
> > > >
> > > > I’m thinking about a functional test suite in the ignite-aws module
> > that
> > > > would pick up a properties file with AWS credentials.
> > > > It wouldn’t be included in any of the TC runs, but someone making
> > changes
> > > > to ignite-aws would be able to run it locally providing their own
> > > > credentials.
> > > > They’d then share the results of their testing together with the
> usual
> > TC
> > > > link.
> > > >
> > > > Stan
> > > >
> > > > From: Stanislav Lukyanov
> > > > Sent: 2 октября 2018 г. 19:08
> > > > To: dev@ignite.apache.org
> > > > Subject: RE: Volunteer needed: AWS Elastic Load Balancer IP
> > > > Findersimplemented
> > > >
> > > > Hi,
> > > >
> > > > I took a look at the code, and I believe the patch needs to be
> > enhanced.
> > > >
> > > > The patch is about adding TcpDiscoveryApplicationElbIpFinder, but it
> > also
> > > > deprecates the existing TcpDiscoveryElbIpFinder
> > > > and replaces it with its copy named TcpDiscoveryClassicElbIpFinder.
> > > >
> > > > I’d really prefer if the existing class were enhanced to support both
> > > > classic ELB and Application ELB.
> > > > If it can’t be done, let’s  use inheritance to reduce the copypaste.
> > > > In any case, let’s avoid deprecating the existing class – I don’t
> think
> > > > that changing the name is that important in this case.
> > > >
> > > > Thanks,
> > > > Stan
> > > >
> > > > From: Denis Magda
> > > > Sent: 26 сентября 2018 г. 18:52
> > > > To: dev
> > > > Subject: Re: Volunteer needed: AWS Elastic Load Balancer IP
> > > > Findersimplemented
> > > >
> > > > Igniters,
> > > >
> > > > Don't we have experts in our networking component? I do believe
> that's
> > a
> > > > small improvement that can be verified and merged quickly.
> > > >
> > > > --
> > > > Denis
> > > >
> > > > On Wed, Sep 26, 2018 at 8:50 AM Dmitriy Pavlov <
> dpavlov.spb@gmail.com>
> > > > wrote:
> > > >
> > > > > Igniters, Friendly reminder.
> > > > >
> > > > > Denis, could you advice how to proceed in this case? It seems
> feature
> > > can
> > > > > be useful, but committers are not involved in a review.
> > > > >
> > > > > Should we send a proposal with lazy consensus and automatically
> > merge?
> > > > Any
> > > > > alternative ideas on how to proceed with issues stuck in the review
> > > > process
> > > > > are strongly appreciated.
> > > > >
> > > > > Sincerely,
> > > > > Dmitriy Pavlov
> > > > >
> > > > > ср, 15 авг. 2018 г. в 19:38, Dmitriy Pavlov <dpavlov.spb@gmail.com
> >:
> > > > >
> > > > > > Hi Igniters,
> > > > > >
> > > > > > I took a look at the proposed contribution
> > > > > > https://issues.apache.org/jira/browse/IGNITE-8617 and PR
> > > > > > https://github.com/apache/ignite/pull/4076.
> > > > > >
> > > > > > I found this change reasonable and I can update code according
to
> > > code
> > > > > > style.
> > > > > >
> > > > > > But I would ask someone from the community with experience with
> AWS
> > > ELB
> > > > > to
> > > > > > join the review. It would also benefit us if someone could try
to
> > > run a
> > > > > > cluster with AWS ELB IP Finder.
> > > > > >
> > > > > > Sincerely,
> > > > > > Dmitriy Pavlov
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message