You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Denis Garus <ga...@gmail.com> on 2019/10/11 06:40:25 UTC
Review needed for IGNITE-11410 Sandbox for user-defined code
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
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Denis Garus <ga...@gmail.com>.
Hello, Igniters!
Alex Plehanov did the review, and I've reworked the implementation of the
issue [1] that is part of the IEP-38 [2].
Could somebody else do a review of the PR [3]?
Alex, thank you for the review!
1. https://issues.apache.org/jira/browse/IGNITE-11410
2. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
3. https://github.com/apache/ignite/pull/6707
чт, 17 окт. 2019 г. в 16:21, Denis Garus <ga...@gmail.com>:
> Hello, Pavel!
>
> Thank you for the feedback!
>
> I've created IEP-38 that describes the Ignite Sandbox [1].
>
> Yes, the issue requires documentation (there is the flag "Docs equired"),
> but common practice is to write documentation in the end.
>
> >> 1) Why do you run resource injection through security and how it tested?
> >> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
> *methods?
> >> These methods are needed only for internal node processes.
> >> 4) There are suspicious security checks at:
> >> CacheOperationContext:37
> >> GridCacheDefaultAffinityKeyMapper:86
> >> PageMemoryImpl:874
> >> I'm not following why they are needed.
>
> These questions have a common answer.
> A user-defined code can call any operation through the public API of
> Ignite. But he may don't have permissions to execute this operation
> successfully.
> For example, to put a value into a cache, it requires permissions for
> accessing to reflection API and reading system property
> IGNITE_ALLOW_ATOMIC_OPS_IN_TX.
> In that case, we have to use AccessController#doPrirvelged call to exclude
> a user-defined code from checking of permissions.
> SecurityUtils#doPriveleged does calling AccessController#doPrirvelged a
> more convenient way.
>
> >> 3) Have you tested security if a compute job is canceled?
> You are right; we should add a test for the cancel case.
> But, for now, we have the issue [2] with the current SecurityContext for
> the canceling of ComputeJob.
>
> 1. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
> 2. https://issues.apache.org/jira/browse/IGNITE-12300
>
> пн, 14 окт. 2019 г. в 16:16, Pavel Kovalenko <jo...@gmail.com>:
>
>> Denis,
>>
>> The idea of having a sandbox for running a user-defined code is useful,
>> but
>> I don't fully understand the implementation approach.
>> There is no detailed description of the ticket about what public API
>> methods or configuration parameters should be covered.
>> There is no description of what have done in initial PR and how.
>> First of all, there should be an umbrella ticket that should contain all
>> public API points and configuration parameters where used defined code may
>> be run.
>> Without a full list of all possible user-defined code injections, we can't
>> track what was covered and where are a possible security lacks.
>> I've checked PR and I have the following questions:
>> 1) Why do you run resource injection through security and how it tested?
>> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
>> *methods?
>> These methods are needed only for internal node processes.
>> 3) Have you tested security if a compute job is canceled?
>> 4) There are suspicious security checks at:
>> CacheOperationContext:37
>> GridCacheDefaultAffinityKeyMapper:86
>> PageMemoryImpl:874
>> I'm not following why they are needed.
>>
>>
>>
>>
>> пн, 14 окт. 2019 г. в 12:19, Anton Vinogradov <av...@apache.org>:
>>
>> > Fully agree with the benchmark's importance.
>> > Currently, we're not able to perform proper benchmarking.
>> > Slava, Is it possible to ask you to check the solution using GridGain's
>> > benchmarking environment?
>> >
>> > On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <
>> > slava.koptilin@gmail.com>
>> > wrote:
>> >
>> > > 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 <av...@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.koptilin@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 <ga...@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.koptilin@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.d.g@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.koptilin@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 <
>> av@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.d.g@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
>> > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Denis Garus <ga...@gmail.com>.
Hello, Pavel!
Thank you for the feedback!
I've created IEP-38 that describes the Ignite Sandbox [1].
Yes, the issue requires documentation (there is the flag "Docs equired"),
but common practice is to write documentation in the end.
>> 1) Why do you run resource injection through security and how it tested?
>> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
*methods?
>> These methods are needed only for internal node processes.
>> 4) There are suspicious security checks at:
>> CacheOperationContext:37
>> GridCacheDefaultAffinityKeyMapper:86
>> PageMemoryImpl:874
>> I'm not following why they are needed.
These questions have a common answer.
A user-defined code can call any operation through the public API of
Ignite. But he may don't have permissions to execute this operation
successfully.
For example, to put a value into a cache, it requires permissions for
accessing to reflection API and reading system property
IGNITE_ALLOW_ATOMIC_OPS_IN_TX.
In that case, we have to use AccessController#doPrirvelged call to exclude
a user-defined code from checking of permissions.
SecurityUtils#doPriveleged does calling AccessController#doPrirvelged a
more convenient way.
>> 3) Have you tested security if a compute job is canceled?
You are right; we should add a test for the cancel case.
But, for now, we have the issue [2] with the current SecurityContext for
the canceling of ComputeJob.
1. https://cwiki.apache.org/confluence/display/IGNITE/IEP-38%3A+Sandbox
2. https://issues.apache.org/jira/browse/IGNITE-12300
пн, 14 окт. 2019 г. в 16:16, Pavel Kovalenko <jo...@gmail.com>:
> Denis,
>
> The idea of having a sandbox for running a user-defined code is useful, but
> I don't fully understand the implementation approach.
> There is no detailed description of the ticket about what public API
> methods or configuration parameters should be covered.
> There is no description of what have done in initial PR and how.
> First of all, there should be an umbrella ticket that should contain all
> public API points and configuration parameters where used defined code may
> be run.
> Without a full list of all possible user-defined code injections, we can't
> track what was covered and where are a possible security lacks.
> I've checked PR and I have the following questions:
> 1) Why do you run resource injection through security and how it tested?
> 2) Why do you check security at *dumpThreads* and *wrapThreadLoader
> *methods?
> These methods are needed only for internal node processes.
> 3) Have you tested security if a compute job is canceled?
> 4) There are suspicious security checks at:
> CacheOperationContext:37
> GridCacheDefaultAffinityKeyMapper:86
> PageMemoryImpl:874
> I'm not following why they are needed.
>
>
>
>
> пн, 14 окт. 2019 г. в 12:19, Anton Vinogradov <av...@apache.org>:
>
> > Fully agree with the benchmark's importance.
> > Currently, we're not able to perform proper benchmarking.
> > Slava, Is it possible to ask you to check the solution using GridGain's
> > benchmarking environment?
> >
> > On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <
> > slava.koptilin@gmail.com>
> > wrote:
> >
> > > 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 <av...@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.koptilin@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 <ga...@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.koptilin@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.d.g@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.koptilin@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 <
> av@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.d.g@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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Pavel Kovalenko <jo...@gmail.com>.
Denis,
The idea of having a sandbox for running a user-defined code is useful, but
I don't fully understand the implementation approach.
There is no detailed description of the ticket about what public API
methods or configuration parameters should be covered.
There is no description of what have done in initial PR and how.
First of all, there should be an umbrella ticket that should contain all
public API points and configuration parameters where used defined code may
be run.
Without a full list of all possible user-defined code injections, we can't
track what was covered and where are a possible security lacks.
I've checked PR and I have the following questions:
1) Why do you run resource injection through security and how it tested?
2) Why do you check security at *dumpThreads* and *wrapThreadLoader *methods?
These methods are needed only for internal node processes.
3) Have you tested security if a compute job is canceled?
4) There are suspicious security checks at:
CacheOperationContext:37
GridCacheDefaultAffinityKeyMapper:86
PageMemoryImpl:874
I'm not following why they are needed.
пн, 14 окт. 2019 г. в 12:19, Anton Vinogradov <av...@apache.org>:
> Fully agree with the benchmark's importance.
> Currently, we're not able to perform proper benchmarking.
> Slava, Is it possible to ask you to check the solution using GridGain's
> benchmarking environment?
>
> On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <
> slava.koptilin@gmail.com>
> wrote:
>
> > 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 <av...@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.koptilin@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 <ga...@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.koptilin@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 <ga...@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.koptilin@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 <av@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.d.g@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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Anton Vinogradov <av...@apache.org>.
Fully agree with the benchmark's importance.
Currently, we're not able to perform proper benchmarking.
Slava, Is it possible to ask you to check the solution using GridGain's
benchmarking environment?
On Mon, Oct 14, 2019 at 12:07 PM Вячеслав Коптилин <sl...@gmail.com>
wrote:
> 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 <av...@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.koptilin@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 <ga...@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.koptilin@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 <ga...@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.koptilin@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 <av...@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.d.g@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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Вячеслав Коптилин <sl...@gmail.com>.
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 <av...@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.koptilin@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 <ga...@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.koptilin@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 <ga...@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.koptilin@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 <av...@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.d.g@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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Anton Vinogradov <av...@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 Вячеслав Коптилин <sl...@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 <ga...@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.koptilin@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 <ga...@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.koptilin@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 <av...@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.d.g@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
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Вячеслав Коптилин <sl...@gmail.com>.
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 <ga...@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, Вячеслав Коптилин <sl...@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 <ga...@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.koptilin@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 <av...@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 <ga...@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
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Denis Garus <ga...@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, Вячеслав Коптилин <sl...@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 <ga...@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.koptilin@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 <av...@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 <ga...@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
> > > > >
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Вячеслав Коптилин <sl...@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 <ga...@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, Вячеслав Коптилин <sl...@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 <av...@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 <ga...@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
> > > >
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Denis Garus <ga...@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, Вячеслав Коптилин <sl...@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 <av...@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 <ga...@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
> > >
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Вячеслав Коптилин <sl...@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 <av...@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 <ga...@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
> >
>
Re: Review needed for IGNITE-11410 Sandbox for user-defined code
Posted by Anton Vinogradov <av...@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 <ga...@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
>