You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Vahid Hashemian <va...@gmail.com> on 2019/05/03 05:16:13 UTC
Re: [DISCUSS] KIP-419 Safely notify Kafka Connect SourceTask is stopped
Hi Andrew,
Thanks for the KIP. I'm not too familiar with the internals of KC so I hope
you can clarify a couple of things:
- It seems the KIP is proposing a new interface because the existing
"stop()" interface doesn't fully perform what it should ideally be doing.
Is that a fair statement?
- You mentioned the "stop()" interface can be called multiple times.
Would the same thing be true for the proposed interface? Does it matter? Or
there is a guard against that?
- I also agree with Ryan that using a verb sounds more intuitive for an
interface that's supposed to trigger some action.
Regards,
--Vahid
On Thu, Jan 24, 2019 at 9:23 AM Ryanne Dolan <ry...@gmail.com> wrote:
> Ah, I'm sorta wrong -- in the current implementation, restartTask()
> stops the task and starts a *new* task instance with the same task ID.
> (I'm not certain that is clear from the documentation or interfaces,
> or if that may change in the future.)
>
> Ryanne
>
> On Thu, Jan 24, 2019 at 10:25 AM Ryanne Dolan <ry...@gmail.com>
> wrote:
> >
> > Andrew, I believe the task can be started again with start() during the
> stopping and stopped states in your diagram.
> >
> > Ryanne
> >
> > On Thu, Jan 24, 2019, 10:20 AM Andrew Schofield <
> andrew_schofield@live.com wrote:
> >>
> >> I've now added a diagram to illustrate the states of a SourceTask. The
> KIP is essentially trying to give a clear signal to SourceTask when all
> work has stopped. In particular, if a SourceTask has a session to the
> source system that it uses in poll() and commit(), it now has a safe way to
> release this.
> >>
> >> Andrew Schofield
> >> IBM Event Streams
> >>
> >> On 21/01/2019, 10:13, "Andrew Schofield" <an...@live.com>
> wrote:
> >>
> >> Ryanne,
> >> Thanks for your comments. I think my overarching point is that the
> various states of a SourceTask and the transitions between them seem a bit
> loose and that makes it difficult to figure out when the resources held by
> a SourceTask can be safely released. Your "I can't tell from the
> documentation" comment is key here __ Neither could I.
> >>
> >> The problem is that stop() is a signal to stop polling. It's
> basically a request from the framework to the task and it doesn't tell the
> task that it's actually finished. One of the purposes of the KC framework
> is to make life easy for a connector developer and a nice clean "all done
> now" method would help.
> >>
> >> I think I'll add a diagram to illustrate to the KIP.
> >>
> >> Andrew Schofield
> >> IBM Event Streams
> >>
> >> On 18/01/2019, 19:02, "Ryanne Dolan" <ry...@gmail.com> wrote:
> >>
> >> Andrew, do we know whether the SourceTask may be start()ed
> again? If this
> >> is the last call to a SourceTask I suggest we call it close().
> I can't tell
> >> from the documentation.
> >>
> >> Also, do we need this if a SourceTask can keep track of whether
> it was
> >> start()ed since the last stop()?
> >>
> >> Ryanne
> >>
> >>
> >> On Fri, Jan 18, 2019, 12:02 PM Andrew Schofield <
> andrew_schofield@live.com
> >> wrote:
> >>
> >> > Hi,
> >> > I’ve created a new KIP to enhance the SourceTask interface in
> Kafka
> >> > Connect.
> >> >
> >> >
> >> >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-419%3A%2BSafely%2Bnotify%2BKafka%2BConnect%2BSourceTask%2Bis%2Bstopped&data=02%7C01%7C%7Cfa73e281fe0348a2740b08d67f8924b8%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636836624328119778&sdata=v6BU3q3W4Q2RIkdWtHCCn5uCSTF%2BMAnbj%2F%2B2%2Flladco%3D&reserved=0
> >> >
> >> > Comments welcome.
> >> >
> >> > Andrew Schofield
> >> > IBM Event Streams
> >> >
> >> >
> >>
> >>
> >>
> >>
>
--
Thanks!
--Vahid
Re: [DISCUSS] KIP-419 Safely notify Kafka Connect SourceTask is
stopped
Posted by Andrew Schofield <an...@live.com>.
Hi Ryanne,
I also worked around it, but would rather not have needed to. So, I'm in the
position of creating a small KIP that I personally don't need to use right now.
But, I do have a PR and it's really simple, so I reckon it's a no-brainer to
enhance the framework in this small way so future connector implementors
can avoid the slight inconvenience of working out when it's safe to clean up
their resources.
Andrew
On 04/05/2019, 14:27, "Ryanne Dolan" <ry...@gmail.com> wrote:
> That could be a defect in the implementation
That's what I am thinking. I don't think there is anything in the interface
or documentation that would imply that stop() could be called multiple
times, so I wouldn't expect any code to break if you fixed this behavior.
Quite the contrary -- I think most implementers would assume stop would be
called exactly once. Also, I don't think I've observed this behavior myself
and would be surprised if I did.
That said, I believe the problem is not that stop() is called multiple
times, but that poll() etc may still have work to do after the framework
calls stop(). This is a signal to the implementation to stop calling
poll(), not to clean up resources, per se.
I handle this by locking resources that I need to close, and then in stop()
I kick off a thread that waits for the locks to be released, at which point
I can cleanup the resources. I agree that it would be nicer if the
framework handled this for me, but the current API isn't too difficult to
work around.
Ryanne
On Fri, May 3, 2019, 10:45 AM Andrew Schofield <an...@live.com>
wrote:
> Hi Vahid,
> Thanks for taking a look at this KIP.
>
> - The KIP is proposing a new interface because the existing "stop()"
> interface
> isn't called at the end of the SourceTask's existence. It's a signal to the
> task to stop, rather than a signal that it has actually stopped.
> Essentially, if
> the task has resources to clean up, there's no clear point at which to do
> this before
> this KIP. I'm trying to make it a bit easier to write a connector with
> this need.
>
> - The "stop()" interface can be called multiple times which you can see by
> setting
> breakpoints. That could be a defect in the implementation but I think it's
> a bit
> risky changing the timing of that call because some connector somewhere
> might
> cease working. Unlikely, but compatibility is important. Also, it's
> important that
> the stop() signal is noticed and a SourceTask runs on multiple threads so
> it's tricky.
> The new method is called exactly once after everything has quiesced.
>
> - I don't disagree that a verb sounds better but couldn't really think of
> a more
> final alternative to "stop()". That's why I went with "stopped()". Could
> be "cleanup()"
> or "release()". Suggestions are welcome.
>
> Thanks.
> Andrew
>
> On 03/05/2019, 06:16, "Vahid Hashemian" <va...@gmail.com>
> wrote:
>
> Hi Andrew,
>
> Thanks for the KIP. I'm not too familiar with the internals of KC so I
> hope
> you can clarify a couple of things:
>
> - It seems the KIP is proposing a new interface because the existing
> "stop()" interface doesn't fully perform what it should ideally be
> doing.
> Is that a fair statement?
> - You mentioned the "stop()" interface can be called multiple times.
> Would the same thing be true for the proposed interface? Does it
> matter? Or
> there is a guard against that?
> - I also agree with Ryan that using a verb sounds more intuitive
> for an
> interface that's supposed to trigger some action.
>
> Regards,
> --Vahid
>
>
> On Thu, Jan 24, 2019 at 9:23 AM Ryanne Dolan <ry...@gmail.com>
> wrote:
>
> > Ah, I'm sorta wrong -- in the current implementation, restartTask()
> > stops the task and starts a *new* task instance with the same task
> ID.
> > (I'm not certain that is clear from the documentation or interfaces,
> > or if that may change in the future.)
> >
> > Ryanne
> >
> > On Thu, Jan 24, 2019 at 10:25 AM Ryanne Dolan <ryannedolan@gmail.com
> >
> > wrote:
> > >
> > > Andrew, I believe the task can be started again with start()
> during the
> > stopping and stopped states in your diagram.
> > >
> > > Ryanne
> > >
> > > On Thu, Jan 24, 2019, 10:20 AM Andrew Schofield <
> > andrew_schofield@live.com wrote:
> > >>
> > >> I've now added a diagram to illustrate the states of a
> SourceTask. The
> > KIP is essentially trying to give a clear signal to SourceTask when
> all
> > work has stopped. In particular, if a SourceTask has a session to the
> > source system that it uses in poll() and commit(), it now has a safe
> way to
> > release this.
> > >>
> > >> Andrew Schofield
> > >> IBM Event Streams
> > >>
> > >> On 21/01/2019, 10:13, "Andrew Schofield" <
> andrew_schofield@live.com>
> > wrote:
> > >>
> > >> Ryanne,
> > >> Thanks for your comments. I think my overarching point is
> that the
> > various states of a SourceTask and the transitions between them seem
> a bit
> > loose and that makes it difficult to figure out when the resources
> held by
> > a SourceTask can be safely released. Your "I can't tell from the
> > documentation" comment is key here __ Neither could I.
> > >>
> > >> The problem is that stop() is a signal to stop polling. It's
> > basically a request from the framework to the task and it doesn't
> tell the
> > task that it's actually finished. One of the purposes of the KC
> framework
> > is to make life easy for a connector developer and a nice clean "all
> done
> > now" method would help.
> > >>
> > >> I think I'll add a diagram to illustrate to the KIP.
> > >>
> > >> Andrew Schofield
> > >> IBM Event Streams
> > >>
> > >> On 18/01/2019, 19:02, "Ryanne Dolan" <ry...@gmail.com>
> wrote:
> > >>
> > >> Andrew, do we know whether the SourceTask may be start()ed
> > again? If this
> > >> is the last call to a SourceTask I suggest we call it
> close().
> > I can't tell
> > >> from the documentation.
> > >>
> > >> Also, do we need this if a SourceTask can keep track of
> whether
> > it was
> > >> start()ed since the last stop()?
> > >>
> > >> Ryanne
> > >>
> > >>
> > >> On Fri, Jan 18, 2019, 12:02 PM Andrew Schofield <
> > andrew_schofield@live.com
> > >> wrote:
> > >>
> > >> > Hi,
> > >> > I’ve created a new KIP to enhance the SourceTask
> interface in
> > Kafka
> > >> > Connect.
> > >> >
> > >> >
> > >> >
> >
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-419%3A%2BSafely%2Bnotify%2BKafka%2BConnect%2BSourceTask%2Bis%2Bstopped&data=02%7C01%7C%7C01ae755e310a423ca1f808d6cf8680af%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636924573921246068&sdata=qgbFXAfB0ih1%2BQ%2B2KX4dyk6BnYE61H0vvhITAVMUyfw%3D&reserved=0
> > >> >
> > >> > Comments welcome.
> > >> >
> > >> > Andrew Schofield
> > >> > IBM Event Streams
> > >> >
> > >> >
> > >>
> > >>
> > >>
> > >>
> >
>
>
> --
>
> Thanks!
> --Vahid
>
>
>
Re: [DISCUSS] KIP-419 Safely notify Kafka Connect SourceTask is stopped
Posted by Ryanne Dolan <ry...@gmail.com>.
> That could be a defect in the implementation
That's what I am thinking. I don't think there is anything in the interface
or documentation that would imply that stop() could be called multiple
times, so I wouldn't expect any code to break if you fixed this behavior.
Quite the contrary -- I think most implementers would assume stop would be
called exactly once. Also, I don't think I've observed this behavior myself
and would be surprised if I did.
That said, I believe the problem is not that stop() is called multiple
times, but that poll() etc may still have work to do after the framework
calls stop(). This is a signal to the implementation to stop calling
poll(), not to clean up resources, per se.
I handle this by locking resources that I need to close, and then in stop()
I kick off a thread that waits for the locks to be released, at which point
I can cleanup the resources. I agree that it would be nicer if the
framework handled this for me, but the current API isn't too difficult to
work around.
Ryanne
On Fri, May 3, 2019, 10:45 AM Andrew Schofield <an...@live.com>
wrote:
> Hi Vahid,
> Thanks for taking a look at this KIP.
>
> - The KIP is proposing a new interface because the existing "stop()"
> interface
> isn't called at the end of the SourceTask's existence. It's a signal to the
> task to stop, rather than a signal that it has actually stopped.
> Essentially, if
> the task has resources to clean up, there's no clear point at which to do
> this before
> this KIP. I'm trying to make it a bit easier to write a connector with
> this need.
>
> - The "stop()" interface can be called multiple times which you can see by
> setting
> breakpoints. That could be a defect in the implementation but I think it's
> a bit
> risky changing the timing of that call because some connector somewhere
> might
> cease working. Unlikely, but compatibility is important. Also, it's
> important that
> the stop() signal is noticed and a SourceTask runs on multiple threads so
> it's tricky.
> The new method is called exactly once after everything has quiesced.
>
> - I don't disagree that a verb sounds better but couldn't really think of
> a more
> final alternative to "stop()". That's why I went with "stopped()". Could
> be "cleanup()"
> or "release()". Suggestions are welcome.
>
> Thanks.
> Andrew
>
> On 03/05/2019, 06:16, "Vahid Hashemian" <va...@gmail.com>
> wrote:
>
> Hi Andrew,
>
> Thanks for the KIP. I'm not too familiar with the internals of KC so I
> hope
> you can clarify a couple of things:
>
> - It seems the KIP is proposing a new interface because the existing
> "stop()" interface doesn't fully perform what it should ideally be
> doing.
> Is that a fair statement?
> - You mentioned the "stop()" interface can be called multiple times.
> Would the same thing be true for the proposed interface? Does it
> matter? Or
> there is a guard against that?
> - I also agree with Ryan that using a verb sounds more intuitive
> for an
> interface that's supposed to trigger some action.
>
> Regards,
> --Vahid
>
>
> On Thu, Jan 24, 2019 at 9:23 AM Ryanne Dolan <ry...@gmail.com>
> wrote:
>
> > Ah, I'm sorta wrong -- in the current implementation, restartTask()
> > stops the task and starts a *new* task instance with the same task
> ID.
> > (I'm not certain that is clear from the documentation or interfaces,
> > or if that may change in the future.)
> >
> > Ryanne
> >
> > On Thu, Jan 24, 2019 at 10:25 AM Ryanne Dolan <ryannedolan@gmail.com
> >
> > wrote:
> > >
> > > Andrew, I believe the task can be started again with start()
> during the
> > stopping and stopped states in your diagram.
> > >
> > > Ryanne
> > >
> > > On Thu, Jan 24, 2019, 10:20 AM Andrew Schofield <
> > andrew_schofield@live.com wrote:
> > >>
> > >> I've now added a diagram to illustrate the states of a
> SourceTask. The
> > KIP is essentially trying to give a clear signal to SourceTask when
> all
> > work has stopped. In particular, if a SourceTask has a session to the
> > source system that it uses in poll() and commit(), it now has a safe
> way to
> > release this.
> > >>
> > >> Andrew Schofield
> > >> IBM Event Streams
> > >>
> > >> On 21/01/2019, 10:13, "Andrew Schofield" <
> andrew_schofield@live.com>
> > wrote:
> > >>
> > >> Ryanne,
> > >> Thanks for your comments. I think my overarching point is
> that the
> > various states of a SourceTask and the transitions between them seem
> a bit
> > loose and that makes it difficult to figure out when the resources
> held by
> > a SourceTask can be safely released. Your "I can't tell from the
> > documentation" comment is key here __ Neither could I.
> > >>
> > >> The problem is that stop() is a signal to stop polling. It's
> > basically a request from the framework to the task and it doesn't
> tell the
> > task that it's actually finished. One of the purposes of the KC
> framework
> > is to make life easy for a connector developer and a nice clean "all
> done
> > now" method would help.
> > >>
> > >> I think I'll add a diagram to illustrate to the KIP.
> > >>
> > >> Andrew Schofield
> > >> IBM Event Streams
> > >>
> > >> On 18/01/2019, 19:02, "Ryanne Dolan" <ry...@gmail.com>
> wrote:
> > >>
> > >> Andrew, do we know whether the SourceTask may be start()ed
> > again? If this
> > >> is the last call to a SourceTask I suggest we call it
> close().
> > I can't tell
> > >> from the documentation.
> > >>
> > >> Also, do we need this if a SourceTask can keep track of
> whether
> > it was
> > >> start()ed since the last stop()?
> > >>
> > >> Ryanne
> > >>
> > >>
> > >> On Fri, Jan 18, 2019, 12:02 PM Andrew Schofield <
> > andrew_schofield@live.com
> > >> wrote:
> > >>
> > >> > Hi,
> > >> > I’ve created a new KIP to enhance the SourceTask
> interface in
> > Kafka
> > >> > Connect.
> > >> >
> > >> >
> > >> >
> >
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-419%3A%2BSafely%2Bnotify%2BKafka%2BConnect%2BSourceTask%2Bis%2Bstopped&data=02%7C01%7C%7C01ae755e310a423ca1f808d6cf8680af%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636924573921246068&sdata=qgbFXAfB0ih1%2BQ%2B2KX4dyk6BnYE61H0vvhITAVMUyfw%3D&reserved=0
> > >> >
> > >> > Comments welcome.
> > >> >
> > >> > Andrew Schofield
> > >> > IBM Event Streams
> > >> >
> > >> >
> > >>
> > >>
> > >>
> > >>
> >
>
>
> --
>
> Thanks!
> --Vahid
>
>
>
Re: [DISCUSS] KIP-419 Safely notify Kafka Connect SourceTask is
stopped
Posted by Andrew Schofield <an...@live.com>.
Hi Vahid,
Thanks for taking a look at this KIP.
- The KIP is proposing a new interface because the existing "stop()" interface
isn't called at the end of the SourceTask's existence. It's a signal to the
task to stop, rather than a signal that it has actually stopped. Essentially, if
the task has resources to clean up, there's no clear point at which to do this before
this KIP. I'm trying to make it a bit easier to write a connector with this need.
- The "stop()" interface can be called multiple times which you can see by setting
breakpoints. That could be a defect in the implementation but I think it's a bit
risky changing the timing of that call because some connector somewhere might
cease working. Unlikely, but compatibility is important. Also, it's important that
the stop() signal is noticed and a SourceTask runs on multiple threads so it's tricky.
The new method is called exactly once after everything has quiesced.
- I don't disagree that a verb sounds better but couldn't really think of a more
final alternative to "stop()". That's why I went with "stopped()". Could be "cleanup()"
or "release()". Suggestions are welcome.
Thanks.
Andrew
On 03/05/2019, 06:16, "Vahid Hashemian" <va...@gmail.com> wrote:
Hi Andrew,
Thanks for the KIP. I'm not too familiar with the internals of KC so I hope
you can clarify a couple of things:
- It seems the KIP is proposing a new interface because the existing
"stop()" interface doesn't fully perform what it should ideally be doing.
Is that a fair statement?
- You mentioned the "stop()" interface can be called multiple times.
Would the same thing be true for the proposed interface? Does it matter? Or
there is a guard against that?
- I also agree with Ryan that using a verb sounds more intuitive for an
interface that's supposed to trigger some action.
Regards,
--Vahid
On Thu, Jan 24, 2019 at 9:23 AM Ryanne Dolan <ry...@gmail.com> wrote:
> Ah, I'm sorta wrong -- in the current implementation, restartTask()
> stops the task and starts a *new* task instance with the same task ID.
> (I'm not certain that is clear from the documentation or interfaces,
> or if that may change in the future.)
>
> Ryanne
>
> On Thu, Jan 24, 2019 at 10:25 AM Ryanne Dolan <ry...@gmail.com>
> wrote:
> >
> > Andrew, I believe the task can be started again with start() during the
> stopping and stopped states in your diagram.
> >
> > Ryanne
> >
> > On Thu, Jan 24, 2019, 10:20 AM Andrew Schofield <
> andrew_schofield@live.com wrote:
> >>
> >> I've now added a diagram to illustrate the states of a SourceTask. The
> KIP is essentially trying to give a clear signal to SourceTask when all
> work has stopped. In particular, if a SourceTask has a session to the
> source system that it uses in poll() and commit(), it now has a safe way to
> release this.
> >>
> >> Andrew Schofield
> >> IBM Event Streams
> >>
> >> On 21/01/2019, 10:13, "Andrew Schofield" <an...@live.com>
> wrote:
> >>
> >> Ryanne,
> >> Thanks for your comments. I think my overarching point is that the
> various states of a SourceTask and the transitions between them seem a bit
> loose and that makes it difficult to figure out when the resources held by
> a SourceTask can be safely released. Your "I can't tell from the
> documentation" comment is key here __ Neither could I.
> >>
> >> The problem is that stop() is a signal to stop polling. It's
> basically a request from the framework to the task and it doesn't tell the
> task that it's actually finished. One of the purposes of the KC framework
> is to make life easy for a connector developer and a nice clean "all done
> now" method would help.
> >>
> >> I think I'll add a diagram to illustrate to the KIP.
> >>
> >> Andrew Schofield
> >> IBM Event Streams
> >>
> >> On 18/01/2019, 19:02, "Ryanne Dolan" <ry...@gmail.com> wrote:
> >>
> >> Andrew, do we know whether the SourceTask may be start()ed
> again? If this
> >> is the last call to a SourceTask I suggest we call it close().
> I can't tell
> >> from the documentation.
> >>
> >> Also, do we need this if a SourceTask can keep track of whether
> it was
> >> start()ed since the last stop()?
> >>
> >> Ryanne
> >>
> >>
> >> On Fri, Jan 18, 2019, 12:02 PM Andrew Schofield <
> andrew_schofield@live.com
> >> wrote:
> >>
> >> > Hi,
> >> > I’ve created a new KIP to enhance the SourceTask interface in
> Kafka
> >> > Connect.
> >> >
> >> >
> >> >
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-419%3A%2BSafely%2Bnotify%2BKafka%2BConnect%2BSourceTask%2Bis%2Bstopped&data=02%7C01%7C%7C01ae755e310a423ca1f808d6cf8680af%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636924573921246068&sdata=qgbFXAfB0ih1%2BQ%2B2KX4dyk6BnYE61H0vvhITAVMUyfw%3D&reserved=0
> >> >
> >> > Comments welcome.
> >> >
> >> > Andrew Schofield
> >> > IBM Event Streams
> >> >
> >> >
> >>
> >>
> >>
> >>
>
--
Thanks!
--Vahid