Folks,
The PR looks good to me. There is one concern - even though the type parameters were placed to IgniteContext by mistake, removing them will break backward compatibility. Are we ok with that? Val, can you comment? 2016-05-21 8:32 GMT-07:00 MaBiao <[hidden email]>: > @agoncharuk <https://github.com/agoncharuk> Would you please help me to > review this PR? > > — > You are receiving this because you were mentioned. > Reply to this email directly or view it on GitHub > <https://github.com/apache/ignite/pull/740#issuecomment-220784081> > |
I think we should change it anyway, because it looks like each new user of
IgniteRDD is confused by this issue. Basically, we require to create separate IgniteContext for each IgniteRDD, which is counterintuitive and is not consistent with Spark APIs. Yes, the upgrade to the next version of Ignite will require a minor code change for existing users, but the current API is wrong and I think we should fix it. Any other opinions? -Val On Tue, May 24, 2016 at 9:29 PM, Alexey Goncharuk < [hidden email]> wrote: > Folks, > > The PR looks good to me. There is one concern - even though the type > parameters were placed to IgniteContext by mistake, removing them will > break backward compatibility. Are we ok with that? > > Val, can you comment? > > 2016-05-21 8:32 GMT-07:00 MaBiao <[hidden email]>: > > > @agoncharuk <https://github.com/agoncharuk> Would you please help me to > > review this PR? > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly or view it on GitHub > > <https://github.com/apache/ignite/pull/740#issuecomment-220784081> > > > |
Yes this is a breaking change however it addresses a real problem. I was
going to pick up the ticket had someone not already fixed this. Thanks for taking care of this. For what it's worth, the original "bug" causes more problems than any breaking change would to fix it. Also Valentin's note about creating multiple IgniteContexts, that doesn't seem to work either if the configuration is different. Would love to see this merged. - Ahmad On Wed, May 25, 2016 at 3:12 AM, Valentin Kulichenko < [hidden email]> wrote: > I think we should change it anyway, because it looks like each new user of > IgniteRDD is confused by this issue. Basically, we require to create > separate IgniteContext for each IgniteRDD, which is counterintuitive and is > not consistent with Spark APIs. > > Yes, the upgrade to the next version of Ignite will require a minor code > change for existing users, but the current API is wrong and I think we > should fix it. > > Any other opinions? > > -Val > > On Tue, May 24, 2016 at 9:29 PM, Alexey Goncharuk < > [hidden email]> wrote: > > > Folks, > > > > The PR looks good to me. There is one concern - even though the type > > parameters were placed to IgniteContext by mistake, removing them will > > break backward compatibility. Are we ok with that? > > > > Val, can you comment? > > > > 2016-05-21 8:32 GMT-07:00 MaBiao <[hidden email]>: > > > > > @agoncharuk <https://github.com/agoncharuk> Would you please help me > to > > > review this PR? > > > > > > — > > > You are receiving this because you were mentioned. > > > Reply to this email directly or view it on GitHub > > > <https://github.com/apache/ignite/pull/740#issuecomment-220784081> > > > > > > -- Ahmad Alkilani Big Data Architect Big Data Services|Platform Services Operations|DreamWorks Animation Office 818.69*5.3374* |
Dmitriy, Yakov, Alexey K.,
What are your thoughts? |
In reply to this post by Ahmad Alkilani
In my view this change fixes a fundamental problem in the initial design,
so I vote to have it merged. We should make it very clear to users that this change is incompatible with previous releases and provide a simple upgrade example in the documentation. On Wed, May 25, 2016 at 10:20 AM, Ahmad Alkilani < [hidden email]> wrote: > Yes this is a breaking change however it addresses a real problem. I was > going to pick up the ticket had someone not already fixed this. Thanks for > taking care of this. > For what it's worth, the original "bug" causes more problems than any > breaking change would to fix it. Also Valentin's note about creating > multiple IgniteContexts, that doesn't seem to work either if the > configuration is different. > > Would love to see this merged. > > - Ahmad > > On Wed, May 25, 2016 at 3:12 AM, Valentin Kulichenko < > [hidden email]> wrote: > > > I think we should change it anyway, because it looks like each new user > of > > IgniteRDD is confused by this issue. Basically, we require to create > > separate IgniteContext for each IgniteRDD, which is counterintuitive and > is > > not consistent with Spark APIs. > > > > Yes, the upgrade to the next version of Ignite will require a minor code > > change for existing users, but the current API is wrong and I think we > > should fix it. > > > > Any other opinions? > > > > -Val > > > > On Tue, May 24, 2016 at 9:29 PM, Alexey Goncharuk < > > [hidden email]> wrote: > > > > > Folks, > > > > > > The PR looks good to me. There is one concern - even though the type > > > parameters were placed to IgniteContext by mistake, removing them will > > > break backward compatibility. Are we ok with that? > > > > > > Val, can you comment? > > > > > > 2016-05-21 8:32 GMT-07:00 MaBiao <[hidden email]>: > > > > > > > @agoncharuk <https://github.com/agoncharuk> Would you please help me > > to > > > > review this PR? > > > > > > > > — > > > > You are receiving this because you were mentioned. > > > > Reply to this email directly or view it on GitHub > > > > <https://github.com/apache/ignite/pull/740#issuecomment-220784081> > > > > > > > > > > > > > -- > Ahmad Alkilani > Big Data Architect > Big Data Services|Platform Services Operations|DreamWorks Animation > > Office 818.69*5.3374* > |
In reply to this post by Alexey Goncharuk
Hi, I wonder if we could add another class to implement the IgniteContext but without type parameter , f.e. IgniteContext2 or something else?
------------------ 原始邮件 ------------------ 发件人: "Alexey Goncharuk";<[hidden email]>; 发送时间: 2016年5月26日(星期四) 凌晨3:10 收件人: "dev"<[hidden email]>; 主题: Re: [apache/ignite] [IGNITE-2929] IgniteContext should not have typeparameters (#740) Dmitriy, Yakov, Alexey K., What are your thoughts? |
And the Apache Spark 2.0 maven repo will be available soon. The spark 2.0 will no longer use SparkContext but SparkSession, so should we also implement the new IgniteSession as the counterpart of SparkSession?
------------------ 原始邮件 ------------------ 发件人: "mabiao";<[hidden email]>; 发送时间: 2016年5月26日(星期四) 上午10:42 收件人: "dev"<[hidden email]>; 主题: 回复: [apache/ignite] [IGNITE-2929] IgniteContext should not have typeparameters (#740) Hi, I wonder if we could add another class to implement the IgniteContext but without type parameter , f.e. IgniteContext2 or something else? ------------------ 原始邮件 ------------------ 发件人: "Alexey Goncharuk";<[hidden email]>; 发送时间: 2016年5月26日(星期四) 凌晨3:10 收件人: "dev"<[hidden email]>; 主题: Re: [apache/ignite] [IGNITE-2929] IgniteContext should not have typeparameters (#740) Dmitriy, Yakov, Alexey K., What are your thoughts? |
Hm, we can use it :) Create properly designed IgniteSession class and
deprecate existing IgniteContext. Thoughts? -Val On Thu, May 26, 2016 at 5:47 AM, mabiao <[hidden email]> wrote: > And the Apache Spark 2.0 maven repo will be available soon. The spark 2.0 > will no longer use SparkContext but SparkSession, so should we also > implement the new IgniteSession as the counterpart of SparkSession? > > > ------------------ 原始邮件 ------------------ > 发件人: "mabiao";<[hidden email]>; > 发送时间: 2016年5月26日(星期四) 上午10:42 > 收件人: "dev"<[hidden email]>; > > 主题: 回复: [apache/ignite] [IGNITE-2929] IgniteContext should not have > typeparameters (#740) > > > > Hi, I wonder if we could add another class to implement the IgniteContext > but without type parameter , f.e. IgniteContext2 or something else? > > > ------------------ 原始邮件 ------------------ > 发件人: "Alexey Goncharuk";<[hidden email]>; > 发送时间: 2016年5月26日(星期四) 凌晨3:10 > 收件人: "dev"<[hidden email]>; > > 主题: Re: [apache/ignite] [IGNITE-2929] IgniteContext should not have > typeparameters (#740) > > > > Dmitriy, Yakov, Alexey K., > > What are your thoughts? > |
Free forum by Nabble | Edit this page |