Hello Anton, > We should avoid heavy merges if possible. Why it should be avoided? To be honest, I don't see any reason for that. Every pull request can be and should be reviewed when it is done and ready to be merged into the epic branch (IEP branch). So, the final review of the entire IEP is just a technical/trivial task, in my opinion.
If I am not mistaken, we are at the stage of preparing a new release (2.8), right? And we are trying to add a new feature that may impact the performance. For example, affinity function, which can be overridden by the end-user, and therefore should be covered by `sandbox`. On the other hand, affinity function is a crucial component that is used very often. Are we really sure that the proposed change does not affect the performance? Do we have a benchmark? Please don't get me wrong, guys. I am not against the feature itself. Moreover, it is a great feature and improvement of security. I just want to say that we need to be sure that we are on the right way of implementing this without affecting other developers. PS: This is just my opinion, which may be wrong. Thanks, S. пн, 14 окт. 2019 г. в 09:09, Anton Vinogradov <a...@apache.org>: > Folks, > > We should avoid heavy merges if possible. > I'm ok with IEP to keep tasks properly, but strictly against all-in-one > "+27000,-18200" merges. > This task implements Sandbox (API + core) which covered by tests and by > some integrations with existing components, which is enough to merge. > The most important thing here is that we will be able to speed-up Sandbox > coverage development once its core menged to the master. > > On Fri, Oct 11, 2019 at 5:41 PM Вячеслав Коптилин < > slava.kopti...@gmail.com> > wrote: > > > Hi Denis, > > > > In my humble opinion, the security (the sandbox feature is about > security, > > right?) either covers all APIs/subsystems or not. > > Security should work always and everywhere otherwise it is not security > :) > > > > > From my point, we should divide the sandbox and features that use it. > > > Also, I added in the main features of Ignite (cache and compute) the > > sandbox calls. > > And at this point, you mixed both in the same pull-request. > > > > > I don't see any problem to have the sandbox in the master branch and > > implement covering for existing and new features if needed. > > On the other hand, this approach leads to ... > > ignite-123 [IEP-X] introduces new cool API > > ignite-124 [IEP-X] improved cool API > > ignite-125 [IEP-X] fixed a bug > > ignite-126 [IEP-X] fixed performance drop > > ignite-127 [IEP-X] Cache API uses new API > > ignite-127 [IEP-X] Compute grid uses new API > > ... > > > > Why should it be a part of the master branch history? All these things > can > > be done on the feature branch, I think. Anyway, it is up to you. > > > > Thanks, > > S. > > > > пт, 11 окт. 2019 г. в 16:31, Denis Garus <garus....@gmail.com>: > > > > > From my point, we should divide the sandbox and features that use it. > > > The sandbox is fully implemented and has needed tests. > > > > > > Also, I added in the main features of Ignite (cache and compute) the > > > sandbox calls. > > > > > > I don't see any problem to have the sandbox in the master branch > > > and implement covering for existing and new features if needed. > > > > > > пт, 11 окт. 2019 г. в 15:21, Вячеслав Коптилин < > slava.kopti...@gmail.com > > >: > > > > > > > Hi Denis, > > > > > > > > Yep, I understand the scope of the ticket, but... I think it is not a > > > good > > > > idea to merge partly implemented feature(s) into the master branch. > > > > Especially, at this moment. We are at the stage of preparing a new > > > release > > > > and I doubt that all improvements, tests (unit tests, integration > > tests, > > > > and performance tests) can be implemented before the release branch > is > > > cut > > > > off. > > > > Personally, I would prefer to create an epic/feature branch for these > > > > activities. In that case, we can implement a feature step by step and > > > merge > > > > it into the master branch once all components are covered. > > > > > > > > > But, sure, we should execute any user-defined code in the sandbox > on > > a > > > > remote node. Feel free to create issues. > > > > will do. > > > > > > > > Thanks, > > > > S. > > > > > > > > пт, 11 окт. 2019 г. в 14:52, Denis Garus <garus....@gmail.com>: > > > > > > > > > Hello, Slava! > > > > > > > > > > The scope of the issue is limited by the following features: > > > > > > > > > > - StreamReceiver for DataStreamer; > > > > > - EntryProcessor; > > > > > - ComputeJob; > > > > > - filter and transformer for ScanQuery. > > > > > > > > > > But, sure, we should execute any user-defined code in the sandbox > on > > a > > > > > remote node. > > > > > Feel free to create issues. > > > > > > > > > > Thanks for the feedback! > > > > > > > > > > пт, 11 окт. 2019 г. в 13:26, Вячеслав Коптилин < > > > slava.kopti...@gmail.com > > > > >: > > > > > > > > > > > Hello Denis, Anton, > > > > > > > > > > > > Could you please clarify the following aspect? Do we need the > same > > > > > > changes/capabilities related to Continuous Queries, Disco > > listeners, > > > > > > CacheStore Factories etc? > > > > > > > > > > > > Thanks, > > > > > > S. > > > > > > > > > > > > пт, 11 окт. 2019 г. в 12:24, Anton Vinogradov <a...@apache.org>: > > > > > > > > > > > > > Folks, > > > > > > > > > > > > > > As a prereviewer, I'd like to say that the solution looks good > to > > > me, > > > > > but > > > > > > > fresh eyes would be good. > > > > > > > > > > > > > > On Fri, Oct 11, 2019 at 9:40 AM Denis Garus < > garus....@gmail.com > > > > > > > > wrote: > > > > > > > > > > > > > > > Hello, Igniters! > > > > > > > > > > > > > > > > I've raised the PR [1] with the sandbox for AI [2]. > > > > > > > > Could somebody review it? > > > > > > > > > > > > > > > > If you have questions and prefer the Slack, I've created the > > > > channel > > > > > > [3]. > > > > > > > > > > > > > > > > 1. https://github.com/apache/ignite/pull/6707 > > > > > > > > 2. https://issues.apache.org/jira/browse/IGNITE-11410 > > > > > > > > 3. https://app.slack.com/client/T4S1WH2J3/CP8JER880 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
