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