Apache Ignite Developers - Legacy Mail Archive

Volunteer needed: AWS Elastic Load Balancer IP Finders implemented

Classic

List

Threaded

12 messages Options

Dmitriy Pavlov

Volunteer needed: AWS Elastic Load Balancer IP Finders implemented

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

Dmitriy Pavlov

Re: Volunteer needed: AWS Elastic Load Balancer IP Finders implemented

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 <[hidden email]>:

> 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
>

dmagda

Re: Volunteer needed: AWS Elastic Load Balancer IP Finders implemented

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 <[hidden email]>
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 <[hidden email]>:
>
> > 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
> >
>

Stanislav Lukyanov

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 <[hidden email]>
wrote:

Stanislav Lukyanov

RE: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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: [hidden email]
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 <[hidden email]>
wrote:

uday kale

Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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 <[hidden email]>
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: [hidden email]
> 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 <[hidden email]>
> 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 <[hidden email]>:
> >
> > > 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
> > >
> >
>
>
>

Dmitriy Pavlov

Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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 <[hidden email]>:

> 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 <[hidden email]>
> 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: [hidden email]
> > 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 <[hidden email]>
> > 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 <[hidden email]>:
> > >
> > > > 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
> > > >
> > >
> >
> >
> >
>

uday kale

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 <[hidden email]> 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 <[hidden email]>:
>
> > 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 <
> [hidden email]>
> > 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: [hidden email]
> > > 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 <[hidden email]>
> > > 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 <[hidden email]>:
> > > >
> > > > > 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
> > > > >
> > > >
> > >
> > >
> > >
> >
>

Stanislav Lukyanov

RE: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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: [hidden email]
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 <[hidden email]> wrote:

uday kale

Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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 <[hidden email]>
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: [hidden email]
> 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 <[hidden email]>
> 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 <[hidden email]>:
> >
> > > 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 <
> > [hidden email]>
> > > 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: [hidden email]
> > > > 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 <
> [hidden email]>
> > > > 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 <[hidden email]
> >:
> > > > >
> > > > > > 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
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> >
>
>

Stanislav Lukyanov

RE: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

OK, even though the name “ELB” applies to both the old and the new IpFinder
in my opinion it isn’t really enough to change the name of the API classes now.

BTW in the docs the features are called Application Load Balancer and Classic Load Balancer,
so the abbreviations would be ALB and CLB, not ApplicationELB and ClassicELB.

To make sure new users are not confused by the names of the classes we need
to make the documentation clear:
1) Explain in the Javadoc of TcpDiscoveryElbIpFinder that it is for the Classic Load Balancers
2) Make sure that Javadocs of TcpDiscoveryElbIpFinder and TcpDiscoveryAlbIpFinder have
each other in “@see” tags
3) Make sure both are described and clearly distinguished at readme.io

Stan

From: uday kale
Sent: 21 декабря 2018 г. 18:30
To: [hidden email]
Subject: Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

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 <[hidden email]>
wrote:

uday kale

Re: Volunteer needed: AWS Elastic Load Balancer IP Findersimplemented

Hi Stanislav,

I agree with your suggestions. Taking a step back I realised that, as a
user of an API, I don't want to spend my time refactoring the code due to
API name changes. I need more concrete reasons to do so. I will update the
PR with the required changes, to code and java-docs, for the review.

Regards,
Uday

On Sun, Dec 23, 2018 at 10:41 PM Stanislav Lukyanov <[hidden email]>
wrote:

> OK, even though the name “ELB” applies to both the old and the new IpFinder
> in my opinion it isn’t really enough to change the name of the API classes
> now.
>
> BTW in the docs the features are called Application Load Balancer and
> Classic Load Balancer,
> so the abbreviations would be ALB and CLB, not ApplicationELB and
> ClassicELB.
>
> To make sure new users are not confused by the names of the classes we need
> to make the documentation clear:
> 1) Explain in the Javadoc of TcpDiscoveryElbIpFinder that it is for the
> Classic Load Balancers
> 2) Make sure that Javadocs of TcpDiscoveryElbIpFinder and
> TcpDiscoveryAlbIpFinder have
> each other in “@see” tags
> 3) Make sure both are described and clearly distinguished at readme.io
>
> Stan
>
> From: uday kale
> Sent: 21 декабря 2018 г. 18:30
> To: [hidden email]
> Subject: Re: Volunteer needed: AWS Elastic Load Balancer IP
> Findersimplemented
>
> 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 <
> [hidden email]>
> 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: [hidden email]
> > 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 <[hidden email]>
> > 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 <[hidden email]>:
> > >
> > > > 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 <
> > > [hidden email]>
> > > > 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: [hidden email]
> > > > > 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 <
> > [hidden email]>
> > > > > 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 <
> [hidden email]
> > >:
> > > > > >
> > > > > > > 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
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
> >
>
>