Re: Re[2]: Google Guava in Ignite 3
Alexander, Feel free to start voting. Please, refer to this discussion in order to avoid a new flaming thread in the voting topic. On Thu, Sep 2, 2021 at 6:04 PM Alexander Polovtcev wrote: > > Andrey, > > > It just doesn't work. If there is precedent of usage of Guava methods > then somebody will use other methods. > > Maybe you are right. I would suggest a vote on whether we should allow Guava > methods in the codebase or not. Let's do that in a separate thread? In the > meantime we will prohibit using Guava until the voting is complete. > > On Thu, Sep 2, 2021 at 3:58 PM Andrey Gura wrote: >> >> > So I would propose to allow at least *some* methods that we consider >> > useful, while disallowing everything else. >> >> It just doesn't work. If there is precedent of usage of Guava methods >> then somebody will use other methods. >> >> > I think that the benefits of using Guava methods instead of copy-pasting >> > them are quite obvious: you don't have to copy-paste code and support it >> >> I don't understand why somebody did such a copy-paste, but I believe >> that nobody supports this code. So, from my point of view, it isn't a >> problem. >> >> On Thu, Sep 2, 2021 at 2:52 PM Alexander Polovtcev >> wrote: >> > >> > Andrey, >> > I think that the benefits of using Guava methods instead of copy-pasting >> > them are quite obvious: you don't have to copy-paste code and support it. >> > I also find this situation quite strange: we have a dependency and >> > copy-paste code from it instead of using it directly. >> > >> > >> > On Thu, Sep 2, 2021 at 1:58 PM Andrey Mashenkov >> > wrote: >> >> >> >> Alex, >> >> >> >> As I understand we agree to shade Guava transitive dependency and >> >> you said a 'maven-shade-plugin' can drop unused Guava methods to reduce >> >> the >> >> footprint of Ignite jar. >> >> >> >> At this point, there is no difference between copy-pasting Guava method to >> >> Ignite and use Guava one. >> >> The resulted jar will have one copy of such method, but in the second >> >> case, >> >> we have to care about compatibility when the transitive Guava dependency >> >> will be updated to a new version. >> >> >> >> So, I see no benefits from using Guava directly. >> >> >> >> >> >> On Thu, Sep 2, 2021 at 11:56 AM Alexander Polovtcev >> >> >> >> wrote: >> >> >> >> > Hi, Andrey! >> >> > I mostly agree with your proposal, but, since we already have some >> >> > copy-paste in our code, can we at least use Guava to remove it? So I >> >> > would >> >> > propose to allow at least *some* methods that we consider useful, while >> >> > disallowing everything else. I understand that it may be difficult to >> >> > formalize, so maybe we can create some kind of a whitelist of Guava >> >> > methods? What do you think? >> >> > >> >> > On Mon, Aug 30, 2021 at 2:54 PM Andrey Gura wrote: >> >> > >> >> > > Follow up >> >> > > >> >> > > On Mon, Aug 23, 2021 at 1:22 PM Andrey Gura wrote: >> >> > > > >> >> > > > Igniters, >> >> > > > >> >> > > > What I actually didn't understand from this thread: Is Guava allowed >> >> > > > in production code of Ignite 3 modules (not dependencies like >> >> > > > Calcite)? >> >> > > > >> >> > > > While we agree with using shading I don't see any arguments about >> >> > > > using Guava library in our code base except for the fact that we >> >> > > > have >> >> > > > some copy-paste of Guava code in the project. >> >> > > > >> >> > > > Guava is rich enough library and it has both advantages and >> >> > > > disadvantages. Ignite code base always strived to be not overloaded >> >> > > > and minimalistic. For example we usually use immutability very rare, >> >> > > > we try to avoid complex and overloaded APIs on hot path, we try >> >> > > > reduce >> >> > > > GC pressure, etc. In turn, Guava is some sense forces using of >> >> > > > immutable collections, has a lot of utility methods which produces >> >> > > > temp object (yes, I don't trust to escap analysys), etc. Rich set of >> >> > > > collections and utility functions will also lead to different >> >> > > > programming styles during Ignite development. I can't predict it, >> >> > > > but >> >> > > > it is usual enough when some developers force immutablity while >> >> > > > others >> >> > > > remove static type declaration and replace it by var :) >> >> > > > >> >> > > > This is a common pratice to reduce language possibilies subset and >> >> > > > limiting using of some libraries in order to provide more or less >> >> > > > unified approach to coding. So I propose to disallow using Guava in >> >> > > > our code base. >> >> > > > >> >> > > > WDYT? >> >> > > > >> >> > > > >> >> > > > On Fri, Aug 20, 2021 at 7:37 PM Alexander Polovtcev >> >> > > > wrote: >> >> > > > > >> >> > > > > Guys, >> >> > > > > >> >> > > > > Thanks again for your responses. I've created a ticket about using >> >> > the >> >> > > > > Shade plugin in order to understand if it is possible to shade and >> >> > > minimize >> >> > > > > Guava,
Re: [Announcement] Apache Ignite 2.11 Code Freeze started
Folks, Sorry for the reminder :-) This issue [1] still blocks the release and the development. There is a week that has passed, however, the issue is still not resolved and cannot be validated within a reasonable time frame. Trust is a good thing and all the community members should trust each other, but we still have the RTC procedure and I don't think we should refuse оf it just because of trust and all of us are highly qualified engineers. This is my humble opinion, but I don't think that this issue [1] is a good candidate to include the release since additional checks are still required. Things like this can quickly turn into dead code (like the AWS and GCE modules do) and it's better to add additional tests for it prior to release them. So here is the plan: - revert [1] from the release 2.11 branch (I'll do it tomorrow) - commit the [1] to the master branch (success build is enough verification for this at this moment) - create and configure TC with a technical azure account to test such module - initiate a discussion to move Azure, AWS, GCE modules to the ignite-extension project - do everything above prior to the 2.12 release. [1] https://issues.apache.org/jira/browse/IGNITE-15388 On Fri, 3 Sept 2021 at 18:36, Pavel Pereslegin wrote: > > Atri, > > > Anyone with azure account should be able to test it > > I checked the test included in the module with an Azure account (as I > mentioned in Jira), it passes successfully. But I'm not sure if this > is enough. > > пт, 3 сент. 2021 г. в 17:48, Nikolay Izhikov : > > > > Atri. > > > > Right now code can’t be tested because it doesn’t compile. > > > > > 3 сент. 2021 г., в 17:42, Atri Sharma написал(а): > > > > > > I am not sure why is it can be tested by few members? > > > > > > Anyone with azure account should be able to test it > > > > > > On Fri, 3 Sep 2021, 19:29 Maxim Muzafarov, wrote: > > > > > >> Folks, > > >> > > >> The GCP and AWS should also be moved to extensions, but this is a > > >> discussion for another topic. > > >> > > >> I trust you all :-) > > >> But who can validate the patch [1]? This is a bad practice that only a > > >> few members be able to test the code. > > >> > > >> > > >> [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > >> > > >> On Fri, 3 Sept 2021 at 16:43, Atri Sharma wrote: > > >>> > > >>> It was extensively tested by myself (running the tests on an actual > > >>> Azure > > >>> account and running an Ignite cluster using an Azure account) and Ilya > > >>> (thanks Ilya!) prior to merging it in the core > > >>> > > >>> On Fri, 3 Sep 2021, 18:50 Denis Magda, wrote: > > >>> > > Disagree to exclude this contribution from the 2.11 release. As Atri > > explained, its implementation and testing approach is identical to the > > >> AWS, > > GCP and Kubernetes IP finders. > > If we want to move the modules to extensions and improve the testing > > approach, then it needs to be done for all similar components. > > > > Atri, have you tested the module in the cloud? If it works, then I > > >> don't > > see any reason why we need to have someone else double-checking this > > >> once > > again. This is the community, we need to trust each other. > > > > - > > Denis > > > > On Fri, Sep 3, 2021 at 9:15 AM Atri Sharma wrote: > > > > > I would argue that the module Co exists with the other IP discovery > > modules > > > (such as GCP and AWS), which are part of core. > > > > > > In terms of tests, the azure module has exactly the same type of > > >> tests as > > > the other two modules mentioned above. > > > > > > On Fri, 3 Sep 2021, 17:54 Maxim Muzafarov, > > >> wrote: > > > > > >> Folks, > > >> > > >> The patch [1] for the azure module seems to be ready for review, > > >> however, there are still some questions that exist: > > >> - Do we have a technical account in azure to test the moude on TC? > > >> Manual testing for such changes is really annoying. > > >> - Is there any reason to add these changes to the main Apache > > >> Ignite > > >> project? Why not extensions? > > >> - It seems there is a lack of tests for this module. > > >> > > >> According to the mentions above, what do you think about moving > > >> this > > >> functionality to the next 2.12 release to solve all the questions > > >> without a rush? I propose to revert the changes related to the > > >> azure > > >> functionality from the 2.11 branch. > > >> > > >> > > >> [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > >> > > >> On Fri, 27 Aug 2021 at 20:53, Dmitriy Pavlov > > wrote: > > >>> > > >>> We had a separate discussion with Max and Alex, so I tend to > > >> agree > > that > > >>> both issues are blockers. > > >>> > > >>> Sincerely, > > >>> Dmitriy Pavlov > > >>> > > >>> пт, 27 авг. 2021 г. в 16:41, Alexey Gidaspo
Re: [Announcement] Apache Ignite 2.11 Code Freeze started
Maxim, I thought Pavel has already reported that the tests run fine with a valid Azure account? On Mon, Sep 6, 2021 at 11:10 PM Maxim Muzafarov wrote: > > Folks, > > Sorry for the reminder :-) > > This issue [1] still blocks the release and the development. There is > a week that has passed, however, the issue is still not resolved and > cannot be validated within a reasonable time frame. Trust is a good > thing and all the community members should trust each other, but we > still have the RTC procedure and I don't think we should refuse оf it > just because of trust and all of us are highly qualified engineers. > This is my humble opinion, but I don't think that this issue [1] is a > good candidate to include the release since additional checks are > still required. Things like this can quickly turn into dead code (like > the AWS and GCE modules do) and it's better to add additional tests > for it prior to release them. > > So here is the plan: > > - revert [1] from the release 2.11 branch (I'll do it tomorrow) > - commit the [1] to the master branch (success build is enough > verification for this at this moment) > - create and configure TC with a technical azure account to test such module > - initiate a discussion to move Azure, AWS, GCE modules to the > ignite-extension project > - do everything above prior to the 2.12 release. > > [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > > > On Fri, 3 Sept 2021 at 18:36, Pavel Pereslegin wrote: > > > > Atri, > > > > > Anyone with azure account should be able to test it > > > > I checked the test included in the module with an Azure account (as I > > mentioned in Jira), it passes successfully. But I'm not sure if this > > is enough. > > > > пт, 3 сент. 2021 г. в 17:48, Nikolay Izhikov : > > > > > > Atri. > > > > > > Right now code can’t be tested because it doesn’t compile. > > > > > > > 3 сент. 2021 г., в 17:42, Atri Sharma написал(а): > > > > > > > > I am not sure why is it can be tested by few members? > > > > > > > > Anyone with azure account should be able to test it > > > > > > > > On Fri, 3 Sep 2021, 19:29 Maxim Muzafarov, wrote: > > > > > > > >> Folks, > > > >> > > > >> The GCP and AWS should also be moved to extensions, but this is a > > > >> discussion for another topic. > > > >> > > > >> I trust you all :-) > > > >> But who can validate the patch [1]? This is a bad practice that only a > > > >> few members be able to test the code. > > > >> > > > >> > > > >> [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > > >> > > > >> On Fri, 3 Sept 2021 at 16:43, Atri Sharma wrote: > > > >>> > > > >>> It was extensively tested by myself (running the tests on an actual > > > >>> Azure > > > >>> account and running an Ignite cluster using an Azure account) and Ilya > > > >>> (thanks Ilya!) prior to merging it in the core > > > >>> > > > >>> On Fri, 3 Sep 2021, 18:50 Denis Magda, wrote: > > > >>> > > > Disagree to exclude this contribution from the 2.11 release. As Atri > > > explained, its implementation and testing approach is identical to > > > the > > > >> AWS, > > > GCP and Kubernetes IP finders. > > > If we want to move the modules to extensions and improve the testing > > > approach, then it needs to be done for all similar components. > > > > > > Atri, have you tested the module in the cloud? If it works, then I > > > >> don't > > > see any reason why we need to have someone else double-checking this > > > >> once > > > again. This is the community, we need to trust each other. > > > > > > - > > > Denis > > > > > > On Fri, Sep 3, 2021 at 9:15 AM Atri Sharma wrote: > > > > > > > I would argue that the module Co exists with the other IP discovery > > > modules > > > > (such as GCP and AWS), which are part of core. > > > > > > > > In terms of tests, the azure module has exactly the same type of > > > >> tests as > > > > the other two modules mentioned above. > > > > > > > > On Fri, 3 Sep 2021, 17:54 Maxim Muzafarov, > > > >> wrote: > > > > > > > >> Folks, > > > >> > > > >> The patch [1] for the azure module seems to be ready for review, > > > >> however, there are still some questions that exist: > > > >> - Do we have a technical account in azure to test the moude on TC? > > > >> Manual testing for such changes is really annoying. > > > >> - Is there any reason to add these changes to the main Apache > > > >> Ignite > > > >> project? Why not extensions? > > > >> - It seems there is a lack of tests for this module. > > > >> > > > >> According to the mentions above, what do you think about moving > > > >> this > > > >> functionality to the next 2.12 release to solve all the questions > > > >> without a rush? I propose to revert the changes related to the > > > >> azure > > > >> functionality from the 2.11 branch. > > > >> > > > >> > > > >> [1] https:
Re: [Announcement] Apache Ignite 2.11 Code Freeze started
Atri, I don't know, currently, I'm not able to verify the results of this execution. For instance, we have the TcpDiscoveryZookeeperIpFinder and it has the corresponding test suite on the TeamCity server, so when I make code changes I can verify these changes on the test environment. How can we be sure that new changes in TCP discovery Ignite's component won't break the Azure module? I don't think we should allow such a practice at all, so the AWS and GCE is not a good example here. Probably we don't know are they work correctly at all because they are don't even used by users. On Mon, 6 Sept 2021 at 21:18, Atri Sharma wrote: > > Maxim, > > I thought Pavel has already reported that the tests run fine with a > valid Azure account? > > On Mon, Sep 6, 2021 at 11:10 PM Maxim Muzafarov wrote: > > > > Folks, > > > > Sorry for the reminder :-) > > > > This issue [1] still blocks the release and the development. There is > > a week that has passed, however, the issue is still not resolved and > > cannot be validated within a reasonable time frame. Trust is a good > > thing and all the community members should trust each other, but we > > still have the RTC procedure and I don't think we should refuse оf it > > just because of trust and all of us are highly qualified engineers. > > This is my humble opinion, but I don't think that this issue [1] is a > > good candidate to include the release since additional checks are > > still required. Things like this can quickly turn into dead code (like > > the AWS and GCE modules do) and it's better to add additional tests > > for it prior to release them. > > > > So here is the plan: > > > > - revert [1] from the release 2.11 branch (I'll do it tomorrow) > > - commit the [1] to the master branch (success build is enough > > verification for this at this moment) > > - create and configure TC with a technical azure account to test such module > > - initiate a discussion to move Azure, AWS, GCE modules to the > > ignite-extension project > > - do everything above prior to the 2.12 release. > > > > [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > > > > > > > On Fri, 3 Sept 2021 at 18:36, Pavel Pereslegin wrote: > > > > > > Atri, > > > > > > > Anyone with azure account should be able to test it > > > > > > I checked the test included in the module with an Azure account (as I > > > mentioned in Jira), it passes successfully. But I'm not sure if this > > > is enough. > > > > > > пт, 3 сент. 2021 г. в 17:48, Nikolay Izhikov : > > > > > > > > Atri. > > > > > > > > Right now code can’t be tested because it doesn’t compile. > > > > > > > > > 3 сент. 2021 г., в 17:42, Atri Sharma написал(а): > > > > > > > > > > I am not sure why is it can be tested by few members? > > > > > > > > > > Anyone with azure account should be able to test it > > > > > > > > > > On Fri, 3 Sep 2021, 19:29 Maxim Muzafarov, wrote: > > > > > > > > > >> Folks, > > > > >> > > > > >> The GCP and AWS should also be moved to extensions, but this is a > > > > >> discussion for another topic. > > > > >> > > > > >> I trust you all :-) > > > > >> But who can validate the patch [1]? This is a bad practice that only > > > > >> a > > > > >> few members be able to test the code. > > > > >> > > > > >> > > > > >> [1] https://issues.apache.org/jira/browse/IGNITE-15388 > > > > >> > > > > >> On Fri, 3 Sept 2021 at 16:43, Atri Sharma wrote: > > > > >>> > > > > >>> It was extensively tested by myself (running the tests on an actual > > > > >>> Azure > > > > >>> account and running an Ignite cluster using an Azure account) and > > > > >>> Ilya > > > > >>> (thanks Ilya!) prior to merging it in the core > > > > >>> > > > > >>> On Fri, 3 Sep 2021, 18:50 Denis Magda, wrote: > > > > >>> > > > > Disagree to exclude this contribution from the 2.11 release. As > > > > Atri > > > > explained, its implementation and testing approach is identical to > > > > the > > > > >> AWS, > > > > GCP and Kubernetes IP finders. > > > > If we want to move the modules to extensions and improve the > > > > testing > > > > approach, then it needs to be done for all similar components. > > > > > > > > Atri, have you tested the module in the cloud? If it works, then I > > > > >> don't > > > > see any reason why we need to have someone else double-checking > > > > this > > > > >> once > > > > again. This is the community, we need to trust each other. > > > > > > > > - > > > > Denis > > > > > > > > On Fri, Sep 3, 2021 at 9:15 AM Atri Sharma wrote: > > > > > > > > > I would argue that the module Co exists with the other IP > > > > > discovery > > > > modules > > > > > (such as GCP and AWS), which are part of core. > > > > > > > > > > In terms of tests, the azure module has exactly the same type of > > > > >> tests as > > > > > the other two modules mentioned above. > > > > > > > > > > On Fri, 3 Sep 2021, 17:54
Re: IEP-78 .NET Thin Client for Ignite 3.0
Val, Would you like me to start the discussion about sync-over-async in Ignite 3 Java APIs, or do you plan to do it yourself? On Fri, Sep 3, 2021 at 10:10 PM Valentin Kulichenko < valentin.kuliche...@gmail.com> wrote: > Makes sense, thanks! > > -Val > > On Fri, Sep 3, 2021 at 2:00 AM Pavel Tupitsyn > wrote: > > > Hi Val, > > > > This is a very good point. > > > > I've looked around blogs, docs, and system APIs, and updated the IEP > > accordingly: > > For Ignite.NET I propose NOT to add sync methods when the actual > > implementation is async: > > - It is easy to consume async APIs in C# with async/await keywords (added > > in 2012 and widely adopted) > > - Most codebases are fully async anyway > > - System APIs and popular libraries follow this direction > > - Sync-over-async is misleading and can affect performance > > > > > > However, I'm not so sure about Java, where async/await are not present, > > overall async usage seems to be rarer, and removing sync methods may > become > > an obstacle for the users in some cases. > > Let's create a separate discussion and see what others think. > > > > Pavel > > > > On Thu, Sep 2, 2021 at 10:32 PM Valentin Kulichenko < > > valentin.kuliche...@gmail.com> wrote: > > > > > Hi Pavel, > > > > > > I've looked at the IEP and the public API - looks good to me. > > > > > > Quick question - do you plan to add sync methods to the interfaces, or > > > you're thinking to only leave async? If the latter, what are the > > arguments > > > for this? The reason I'm asking is that I'm actually thinking about > > > suggesting the same for Java as well (or at least having a discussion > > about > > > this). > > > > > > -Val > > > > > > On Thu, Sep 2, 2021 at 10:08 AM Pavel Tupitsyn > > > wrote: > > > > > > > Igniters, > > > > > > > > Please review the IEP [1] and the PoC [2] for .NET Thin Client in > > Ignite > > > > 3.0. > > > > > > > > [1] > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-78+.NET+Thin+Client > > > > [2] https://github.com/apache/ignite-3/pull/306 > > > > > > > > > >
