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