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: dev@ignite.apache.org 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 <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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
