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 <stanlukya...@gmail.com> 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: 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
