ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpav...@apache.org>
Subject Re: Request for review : IGNITE-3303 Apache Flink Integration - Flink source
Date Tue, 25 Dec 2018 16:38:41 GMT
Hi Saikat,

I'm going to slightly change the code before commit.

First of all, I will add more code style and abbreviation rules related
changes.
Second of all, I add pom.xml variables usages where possible.
And last but not least, I'm going to change the implementation of
IgniteSource.

AFAIK drainTo() method is not blocking so IgniteSource will be continuously
spinning if there are no events coming from a cache. So I'm going to commit
this variant:

while (isRunning) {
    // block here for some time if there is no events from source
    CacheEvent firstEvt = evtBuf.poll(1, TimeUnit.SECONDS);

    if (firstEvt != null)
        evts.add(firstEvt);

    if (evtBuf.drainTo(evts, evtBatchSize) > 0) {
        synchronized (ctx.getCheckpointLock()) {
            for (CacheEvent evt : evts)
                ctx.collect(evt);

            evts.clear();
        }
    }
}


I hope you don't mind. This option allows us both to stop and does not
spin in the while.


Sincerely,

Dmitriy Pavlov


сб, 13 окт. 2018 г. в 20:37, Saikat Maitra <saikat.maitra@gmail.com>:

> Hi Alex ,
>
> Can you please review and let me know if the PR looks good to merge.
>
> I have completed the requested changes.
>
> Regards,
> Saikat
>
> On Mon, Oct 1, 2018 at 9:24 PM Saikat Maitra <saikat.maitra@gmail.com>
> wrote:
>
> > Thank you everyone for reviewing the changes. As discussed I have removed
> > the FlinkIgniteSourceSelfExample and also verified that
> FlinkIgniteSourceSelfTestSuite
> > is part of Ignite Streamers in Team City.
> >
> > Please let me know if any further changes are required.
> >
> > Alex , will you please review and let me know if the PR looks good?
> >
> > Regards,
> > Saikat
> >
> > On Mon, Oct 1, 2018 at 11:58 AM Nikolay Izhikov <nizhikov@apache.org>
> > wrote:
> >
> >> Hello, Andrey.
> >>
> >> Yes, I know it.
> >> I've looked at the PR befor mailing :)
> >>
> >> Do you think we can include this improvement to the 2.7 release?
> >> Do you have time to assist Saikat with TC setup and other tasks?
> >>
> >>
> >> В Пн, 01/10/2018 в 19:54 +0300, Andrey Mashenkov пишет:
> >> > Dmitry, Nikolay,
> >> >
> >> > Ignite-3303 is a new Ignite module and there is no changes related to
> >> core or other existed modules.
> >> > So, PR should not affected existed functional ity and can be safely
> >> merged.
> >> >
> >> > Thanks.
> >> >
> >> >
> >> > пн, 1 окт. 2018 г., 16:04 Dmitriy Pavlov <dpavlov.spb@gmail.com>:
> >> > > Hi Saikat,
> >> > >
> >> > > I don't mind merging to master, but I have concern if it will go to
> >> 2.7. In
> >> > > the separate discussion, we agreed on code freeze should happen
> >> during last
> >> > > weekend, September, 30.
> >> > >
> >> > > So it is now up to community and release manager to decide if fix
> >> should go
> >> > > to the upcoming release. Usually, after the freeze, only bug/test
> >> fixes can
> >> > > be merged to release branch.
> >> > >
> >> > > Hi Nikolay,
> >> > >
> >> > > could you please announce that code freeze happened?
> >> > >
> >> > > Sincerely,
> >> > > Dmitriy Pavlov
> >> > >
> >> > > пн, 1 окт. 2018 г. в 3:58, Saikat Maitra <saikat.maitra@gmail.com>:
> >> > >
> >> > > > Hi Alex, Nicolay
> >> > > >
> >> > > > As discussed with Andrew the changes looks good. Would it be
ok to
> >> merge
> >> > > > this change to master considering the 2.7 release plan?
> >> > > >
> >> > > > Regards,
> >> > > > Saikat
> >> > > >
> >> > > > On Fri, Sep 28, 2018 at 7:15 PM Saikat Maitra <
> >> saikat.maitra@gmail.com>
> >> > > > wrote:
> >> > > >
> >> > > > > Thank you Andrew
> >> > > > >
> >> > > > > Regards,
> >> > > > > Saikat
> >> > > > >
> >> > > > > On Fri, Sep 28, 2018 at 7:00 PM Andrey Mashenkov <
> >> > > > > andrey.mashenkov@gmail.com> wrote:
> >> > > > >
> >> > > > >> Hi Saikat,
> >> > > > >>
> >> > > > >> Sorry for late answer. I've checked changes a day ago.
Now,
> >> looks good.
> >> > > > >> Hope, it will be merged soon.
> >> > > > >>
> >> > > > >> Alex, would you please merge PR to master.
> >> > > > >>
> >> > > > >> сб, 29 сент. 2018 г., 2:29 Saikat Maitra <
> >> saikat.maitra@gmail.com>:
> >> > > > >>
> >> > > > >> > Hi Andrew,
> >> > > > >> >
> >> > > > >> > I have updated the changes.
> >> > > > >> >
> >> > > > >> > Can you please review and share feedback.
> >> > > > >> >
> >> > > > >> > Regards
> >> > > > >> > Saikat
> >> > > > >> >
> >> > > > >> > On Sat, Sep 22, 2018 at 2:23 PM Saikat Maitra <
> >> > > > saikat.maitra@gmail.com>
> >> > > > >> > wrote:
> >> > > > >> >
> >> > > > >> > > Hi Andrew
> >> > > > >> > >
> >> > > > >> > >
> >> > > > >> > > I have updated the changes.
> >> > > > >> > >
> >> > > > >> > >
> >> > > > >> > > Can you please review and share feedback.
> >> > > > >> > >
> >> > > > >> > >
> >> > > > >> > > Regards
> >> > > > >> > > Saikat
> >> > > > >> > >
> >> > > > >> > >
> >> > > > >> > > On Wed, Sep 19, 2018 at 8:11 PM, Saikat Maitra
<
> >> > > > >> saikat.maitra@gmail.com>
> >> > > > >> > > wrote:
> >> > > > >> > >
> >> > > > >> > >> Hi Andrew,
> >> > > > >> > >>
> >> > > > >> > >> I have updated the tests and also added
java docs.
> >> > > > >> > >>
> >> > > > >> > >> Can you please review and share feedback.
> >> > > > >> > >>
> >> > > > >> > >>
> >> > > > >> > >> Regards
> >> > > > >> > >> Saikat
> >> > > > >> > >>
> >> > > > >> > >>
> >> > > > >> > >>
> >> > > > >> > >>
> >> > > > >> > >> On Sun, Sep 16, 2018 at 11:53 AM, Saikat
Maitra <
> >> > > > >> > saikat.maitra@gmail.com>
> >> > > > >> > >> wrote:
> >> > > > >> > >>
> >> > > > >> > >>> Hi Andrew,
> >> > > > >> > >>>
> >> > > > >> > >>> I have updated the tests and also
added java docs.
> >> > > > >> > >>>
> >> > > > >> > >>> Please review and share feedback.
> >> > > > >> > >>>
> >> > > > >> > >>> Regards
> >> > > > >> > >>> Saikat
> >> > > > >> > >>>
> >> > > > >> > >>>
> >> > > > >> > >>> On Sat, Sep 8, 2018 at 2:09 PM, Saikat
Maitra <
> >> > > > >> saikat.maitra@gmail.com
> >> > > > >> > >
> >> > > > >> > >>> wrote:
> >> > > > >> > >>>
> >> > > > >> > >>>> Hi Andrew, Alexey
> >> > > > >> > >>>>
> >> > > > >> > >>>> I have incorporated the review
changes.
> >> > > > >> > >>>>
> >> > > > >> > >>>> I have also refactored the CacheEventSerializer
class
> and
> >> moved
> >> > > > it
> >> > > > >> to
> >> > > > >> > >>>> test folder because it is used
only in the
> >> > > > >> > FlinkIgniteSourceSelfExample and
> >> > > > >> > >>>> not required for IgniteSource.
> >> > > > >> > >>>>
> >> > > > >> > >>>> Build links
> >> > > > >> > https://ci.ignite.apache.org/viewLog.html?buildId=1821778&;
> >> > > > >> > >>>>
> >> > > > >> > >>>>
> >> https://ci.ignite.apache.org/viewLog.html?buildId=1821774&;
> >> > > > >> > >>>>
> >> > > > >> > >>>> Please review and share feedback.
> >> > > > >> > >>>>
> >> > > > >> > >>>> Regards
> >> > > > >> > >>>> Saikat
> >> > > > >> > >>>>
> >> > > > >> > >>>> On Tue, Sep 4, 2018 at 9:57 PM,
Saikat Maitra <
> >> > > > >> > saikat.maitra@gmail.com>
> >> > > > >> > >>>> wrote:
> >> > > > >> > >>>>
> >> > > > >> > >>>>> Hi Alexey,
> >> > > > >> > >>>>>
> >> > > > >> > >>>>> Thank you for reviewing the
changes and sharing
> >> feedback, I am
> >> > > > >> > >>>>> updating the PR. I will share
the changes shortly.
> >> > > > >> > >>>>>
> >> > > > >> > >>>>> Regards,
> >> > > > >> > >>>>> Saikat
> >> > > > >> > >>>>>
> >> > > > >> > >>>>> On Tue, Sep 4, 2018 at 10:59
AM, Alexey Goncharuk <
> >> > > > >> > >>>>> alexey.goncharuk@gmail.com>
wrote:
> >> > > > >> > >>>>>
> >> > > > >> > >>>>>> Hello Saikat,
> >> > > > >> > >>>>>>
> >> > > > >> > >>>>>> I see a few fellow Igniters
added some comments to
> your
> >> PR
> >> > > > >> > (including
> >> > > > >> > >>>>>> me).
> >> > > > >> > >>>>>> I believe the PR can be
merged after you address them.
> >> > > > >> > >>>>>>
> >> > > > >> > >>>>>> Thanks,
> >> > > > >> > >>>>>> AG
> >> > > > >> > >>>>>>
> >> > > > >> > >>>>>> пт, 31 авг. 2018
г. в 3:11, Saikat Maitra <
> >> > > > >> saikat.maitra@gmail.com
> >> > > > >> > >:
> >> > > > >> > >>>>>>
> >> > > > >> > >>>>>> > Thank you, Denis
> >> > > > >> > >>>>>> >
> >> > > > >> > >>>>>> > Regards,
> >> > > > >> > >>>>>> > Saikat
> >> > > > >> > >>>>>> >
> >> > > > >> > >>>>>> > On Thu, Aug 30, 2018
at 7:01 PM, Denis Magda <
> >> > > > >> dmagda@apache.org>
> >> > > > >> > >>>>>> wrote:
> >> > > > >> > >>>>>> >
> >> > > > >> > >>>>>> > > Hello Saikat,
> >> > > > >> > >>>>>> > >
> >> > > > >> > >>>>>> > > Hopefully, someone
from the community will review
> >> the
> >> > > > >> changes in
> >> > > > >> > >>>>>> the
> >> > > > >> > >>>>>> > > nearest time.
> >> > > > >> > >>>>>> > >
> >> > > > >> > >>>>>> > > --
> >> > > > >> > >>>>>> > > Denis
> >> > > > >> > >>>>>> > >
> >> > > > >> > >>>>>> > > On Thu, Aug
30, 2018 at 4:37 PM Saikat Maitra <
> >> > > > >> > >>>>>> saikat.maitra@gmail.com>
> >> > > > >> > >>>>>> > > wrote:
> >> > > > >> > >>>>>> > >
> >> > > > >> > >>>>>> > > > Hello,
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > The changes
for IGNITE-3303 for IgniteSource is
> >> complete.
> >> > > > >> This
> >> > > > >> > >>>>>> will
> >> > > > >> > >>>>>> > help
> >> > > > >> > >>>>>> > > is
> >> > > > >> > >>>>>> > > > streaming
data from Ignite cluster and process,
> >> filter,
> >> > > > >> > >>>>>> transform and
> >> > > > >> > >>>>>> > > > publish
it back to Ignite using IgniteSink or in
> >> any
> >> > > > other
> >> > > > >> > data
> >> > > > >> > >>>>>> sink.
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > I was hoping
if the changes can be approved I
> can
> >> go
> >> > > > ahead
> >> > > > >> > >>>>>> merge the
> >> > > > >> > >>>>>> > > > changes.
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > Regards,
> >> > > > >> > >>>>>> > > > Saikat
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > On Tue,
Aug 28, 2018 at 12:56 AM, Saikat Maitra
> <
> >> > > > >> > >>>>>> > saikat.maitra@gmail.com
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > wrote:
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > > > > Hi
Andrew,
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > > > As
discussed I have incorporated the changes.
> >> Please
> >> > > > >> review
> >> > > > >> > >>>>>> and let
> >> > > > >> > >>>>>> > me
> >> > > > >> > >>>>>> > > > > know
if any changes required.
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > > > Regards,
> >> > > > >> > >>>>>> > > > > Saikat
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > > > On
Mon, Aug 27, 2018 at 1:45 AM, Saikat
> Maitra <
> >> > > > >> > >>>>>> > > saikat.maitra@gmail.com>
> >> > > > >> > >>>>>> > > > > wrote:
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > > >>
Hi,
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
I have updated the PR with additional tests.
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
Please review and share feedback.
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
This PR is related to IgniteSink but allows
> to
> >> stream
> >> > > > >> data
> >> > > > >> > >>>>>> from
> >> > > > >> > >>>>>> > > Ignite.
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
PR
> >> https://github.com/apache/ignite/pull/870/files
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
Review
> >> > > > >> > >>>>>>
> >> https://reviews.ignite.apache.org/ignite/review/IGNT-CR-135
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >>
Regards,
> >> > > > >> > >>>>>> > > > >>
Saikat
> >> > > > >> > >>>>>> > > > >>
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > > >
> >> > > > >> > >>>>>> > > >
> >> > > > >> > >>>>>> > >
> >> > > > >> > >>>>>> >
> >> > > > >> > >>>>>>
> >> > > > >> > >>>>>
> >> > > > >> > >>>>>
> >> > > > >> > >>>>
> >> > > > >> > >>>
> >> > > > >> > >>
> >> > > > >> > >
> >> > > > >> >
> >> > > > >>
> >> > > > >
> >> > > >
> >>
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message