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....@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 <udaygk...@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 < > stanlukya...@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....@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....@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 > > > > > > > > > > > > > > > > > > > > >
