> 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 <andrew_schofi...@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" <vahid.hashem...@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 <ryannedo...@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 <ryannedo...@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_schofi...@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_schofi...@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" <ryannedo...@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_schofi...@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 > > >
