You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by Stephen Boesch <ja...@gmail.com> on 2014/08/05 21:14:14 UTC
Tiny curiosity question on closing the jdbc connection
Within its compute.close method, the JdbcRDD class has this interesting
logic for closing jdbc connection:
try {
if (null != conn && ! stmt.isClosed()) conn.close()
logInfo("closed connection")
} catch {
case e: Exception => logWarning("Exception closing connection", e)
}
Notice that the second check is on stmt having been closed - not on the
connection.
I would wager this were not a simple oversight and there were some
motivation for this logic- curious if anyone would be able to shed some
light? My particular interest is that I have written custom ORM's in jdbc
since late 90's and never did it this way.
Re: Tiny curiosity question on closing the jdbc connection
Posted by Stephen Boesch <ja...@gmail.com>.
Hi yes that callback takes care of it. thanks!
2014-08-05 13:58 GMT-07:00 Cody Koeninger <co...@koeninger.org>:
> The stmt.isClosed just looks like stupidity on my part, no secret
> motivation :) Thanks for noticing it.
>
> As for the leaking in the case of malformed statements, isn't that
> addressed by
>
> context.addOnCompleteCallback{ () => closeIfNeeded() }
>
> or am I misunderstanding?
>
>
> On Tue, Aug 5, 2014 at 3:15 PM, Reynold Xin <rx...@databricks.com> wrote:
>
>> Thanks. Those are definitely great problems to fix!
>>
>>
>>
>> On Tue, Aug 5, 2014 at 1:11 PM, Stephen Boesch <ja...@gmail.com> wrote:
>>
>> > Thanks Reynold, Ted Yu did mention offline and I put in a jira already.
>> > Another small concern: there appears to be no exception handling from
>> the
>> > creation of the prepared statement (line 74) through to the executeQuery
>> > (line 86). In case of error/exception it would seem to be leaking
>> > connections (/statements). If that were the case then I would include a
>> > small patch for the exception trapping in that section of code as well.
>> > BTW I was looking at this code for another reason, not intending to be
>> a
>> > bother ;)
>> >
>> >
>> >
>> >
>> > 2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
>> >
>> > I'm pretty sure it is an oversight. Would you like to submit a pull
>> >> request to fix that?
>> >>
>> >>
>> >>
>> >> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com>
>> >> wrote:
>> >>
>> >>> Within its compute.close method, the JdbcRDD class has this
>> interesting
>> >>> logic for closing jdbc connection:
>> >>>
>> >>>
>> >>> try {
>> >>> if (null != conn && ! stmt.isClosed()) conn.close()
>> >>> logInfo("closed connection")
>> >>> } catch {
>> >>> case e: Exception => logWarning("Exception closing
>> connection",
>> >>> e)
>> >>> }
>> >>>
>> >>> Notice that the second check is on stmt having been closed - not on
>> the
>> >>> connection.
>> >>>
>> >>> I would wager this were not a simple oversight and there were some
>> >>> motivation for this logic- curious if anyone would be able to shed
>> some
>> >>> light? My particular interest is that I have written custom ORM's in
>> >>> jdbc
>> >>> since late 90's and never did it this way.
>> >>>
>> >>
>> >>
>> >
>>
>
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Stephen Boesch <ja...@gmail.com>.
The existing callback does take care of it: within the DAGScheduler there
is a finally block to ensure the callbacks are executed.
try {
val result = job.func(taskContext, rdd.iterator(split, taskContext))
job.listener.taskSucceeded(0, result)
} finally {
taskContext.executeOnCompleteCallbacks()
}
So I have removed that exception handling code from the PR and updated the
JIRA.
2014-08-05 14:01 GMT-07:00 Reynold Xin <rx...@databricks.com>:
> Yes it is. I actually commented on it:
> https://github.com/apache/spark/pull/1792/files#r15840899
>
>
>
> On Tue, Aug 5, 2014 at 1:58 PM, Cody Koeninger <co...@koeninger.org> wrote:
>
>> The stmt.isClosed just looks like stupidity on my part, no secret
>> motivation :) Thanks for noticing it.
>>
>> As for the leaking in the case of malformed statements, isn't that
>> addressed by
>>
>> context.addOnCompleteCallback{ () => closeIfNeeded() }
>>
>> or am I misunderstanding?
>>
>>
>> On Tue, Aug 5, 2014 at 3:15 PM, Reynold Xin <rx...@databricks.com> wrote:
>>
>> > Thanks. Those are definitely great problems to fix!
>> >
>> >
>> >
>> > On Tue, Aug 5, 2014 at 1:11 PM, Stephen Boesch <ja...@gmail.com>
>> wrote:
>> >
>> > > Thanks Reynold, Ted Yu did mention offline and I put in a jira
>> already.
>> > > Another small concern: there appears to be no exception handling from
>> the
>> > > creation of the prepared statement (line 74) through to the
>> executeQuery
>> > > (line 86). In case of error/exception it would seem to be leaking
>> > > connections (/statements). If that were the case then I would
>> include a
>> > > small patch for the exception trapping in that section of code as
>> well.
>> > > BTW I was looking at this code for another reason, not intending to
>> be a
>> > > bother ;)
>> > >
>> > >
>> > >
>> > >
>> > > 2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
>> > >
>> > > I'm pretty sure it is an oversight. Would you like to submit a pull
>> > >> request to fix that?
>> > >>
>> > >>
>> > >>
>> > >> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com>
>> > >> wrote:
>> > >>
>> > >>> Within its compute.close method, the JdbcRDD class has this
>> interesting
>> > >>> logic for closing jdbc connection:
>> > >>>
>> > >>>
>> > >>> try {
>> > >>> if (null != conn && ! stmt.isClosed()) conn.close()
>> > >>> logInfo("closed connection")
>> > >>> } catch {
>> > >>> case e: Exception => logWarning("Exception closing
>> connection",
>> > >>> e)
>> > >>> }
>> > >>>
>> > >>> Notice that the second check is on stmt having been closed - not on
>> > the
>> > >>> connection.
>> > >>>
>> > >>> I would wager this were not a simple oversight and there were some
>> > >>> motivation for this logic- curious if anyone would be able to shed
>> some
>> > >>> light? My particular interest is that I have written custom ORM's
>> in
>> > >>> jdbc
>> > >>> since late 90's and never did it this way.
>> > >>>
>> > >>
>> > >>
>> > >
>> >
>>
>
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Reynold Xin <rx...@databricks.com>.
Yes it is. I actually commented on it:
https://github.com/apache/spark/pull/1792/files#r15840899
On Tue, Aug 5, 2014 at 1:58 PM, Cody Koeninger <co...@koeninger.org> wrote:
> The stmt.isClosed just looks like stupidity on my part, no secret
> motivation :) Thanks for noticing it.
>
> As for the leaking in the case of malformed statements, isn't that
> addressed by
>
> context.addOnCompleteCallback{ () => closeIfNeeded() }
>
> or am I misunderstanding?
>
>
> On Tue, Aug 5, 2014 at 3:15 PM, Reynold Xin <rx...@databricks.com> wrote:
>
> > Thanks. Those are definitely great problems to fix!
> >
> >
> >
> > On Tue, Aug 5, 2014 at 1:11 PM, Stephen Boesch <ja...@gmail.com>
> wrote:
> >
> > > Thanks Reynold, Ted Yu did mention offline and I put in a jira already.
> > > Another small concern: there appears to be no exception handling from
> the
> > > creation of the prepared statement (line 74) through to the
> executeQuery
> > > (line 86). In case of error/exception it would seem to be leaking
> > > connections (/statements). If that were the case then I would include
> a
> > > small patch for the exception trapping in that section of code as well.
> > > BTW I was looking at this code for another reason, not intending to
> be a
> > > bother ;)
> > >
> > >
> > >
> > >
> > > 2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
> > >
> > > I'm pretty sure it is an oversight. Would you like to submit a pull
> > >> request to fix that?
> > >>
> > >>
> > >>
> > >> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com>
> > >> wrote:
> > >>
> > >>> Within its compute.close method, the JdbcRDD class has this
> interesting
> > >>> logic for closing jdbc connection:
> > >>>
> > >>>
> > >>> try {
> > >>> if (null != conn && ! stmt.isClosed()) conn.close()
> > >>> logInfo("closed connection")
> > >>> } catch {
> > >>> case e: Exception => logWarning("Exception closing
> connection",
> > >>> e)
> > >>> }
> > >>>
> > >>> Notice that the second check is on stmt having been closed - not on
> > the
> > >>> connection.
> > >>>
> > >>> I would wager this were not a simple oversight and there were some
> > >>> motivation for this logic- curious if anyone would be able to shed
> some
> > >>> light? My particular interest is that I have written custom ORM's
> in
> > >>> jdbc
> > >>> since late 90's and never did it this way.
> > >>>
> > >>
> > >>
> > >
> >
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Cody Koeninger <co...@koeninger.org>.
The stmt.isClosed just looks like stupidity on my part, no secret
motivation :) Thanks for noticing it.
As for the leaking in the case of malformed statements, isn't that
addressed by
context.addOnCompleteCallback{ () => closeIfNeeded() }
or am I misunderstanding?
On Tue, Aug 5, 2014 at 3:15 PM, Reynold Xin <rx...@databricks.com> wrote:
> Thanks. Those are definitely great problems to fix!
>
>
>
> On Tue, Aug 5, 2014 at 1:11 PM, Stephen Boesch <ja...@gmail.com> wrote:
>
> > Thanks Reynold, Ted Yu did mention offline and I put in a jira already.
> > Another small concern: there appears to be no exception handling from the
> > creation of the prepared statement (line 74) through to the executeQuery
> > (line 86). In case of error/exception it would seem to be leaking
> > connections (/statements). If that were the case then I would include a
> > small patch for the exception trapping in that section of code as well.
> > BTW I was looking at this code for another reason, not intending to be a
> > bother ;)
> >
> >
> >
> >
> > 2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
> >
> > I'm pretty sure it is an oversight. Would you like to submit a pull
> >> request to fix that?
> >>
> >>
> >>
> >> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com>
> >> wrote:
> >>
> >>> Within its compute.close method, the JdbcRDD class has this interesting
> >>> logic for closing jdbc connection:
> >>>
> >>>
> >>> try {
> >>> if (null != conn && ! stmt.isClosed()) conn.close()
> >>> logInfo("closed connection")
> >>> } catch {
> >>> case e: Exception => logWarning("Exception closing connection",
> >>> e)
> >>> }
> >>>
> >>> Notice that the second check is on stmt having been closed - not on
> the
> >>> connection.
> >>>
> >>> I would wager this were not a simple oversight and there were some
> >>> motivation for this logic- curious if anyone would be able to shed some
> >>> light? My particular interest is that I have written custom ORM's in
> >>> jdbc
> >>> since late 90's and never did it this way.
> >>>
> >>
> >>
> >
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Reynold Xin <rx...@databricks.com>.
Thanks. Those are definitely great problems to fix!
On Tue, Aug 5, 2014 at 1:11 PM, Stephen Boesch <ja...@gmail.com> wrote:
> Thanks Reynold, Ted Yu did mention offline and I put in a jira already.
> Another small concern: there appears to be no exception handling from the
> creation of the prepared statement (line 74) through to the executeQuery
> (line 86). In case of error/exception it would seem to be leaking
> connections (/statements). If that were the case then I would include a
> small patch for the exception trapping in that section of code as well.
> BTW I was looking at this code for another reason, not intending to be a
> bother ;)
>
>
>
>
> 2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
>
> I'm pretty sure it is an oversight. Would you like to submit a pull
>> request to fix that?
>>
>>
>>
>> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com>
>> wrote:
>>
>>> Within its compute.close method, the JdbcRDD class has this interesting
>>> logic for closing jdbc connection:
>>>
>>>
>>> try {
>>> if (null != conn && ! stmt.isClosed()) conn.close()
>>> logInfo("closed connection")
>>> } catch {
>>> case e: Exception => logWarning("Exception closing connection",
>>> e)
>>> }
>>>
>>> Notice that the second check is on stmt having been closed - not on the
>>> connection.
>>>
>>> I would wager this were not a simple oversight and there were some
>>> motivation for this logic- curious if anyone would be able to shed some
>>> light? My particular interest is that I have written custom ORM's in
>>> jdbc
>>> since late 90's and never did it this way.
>>>
>>
>>
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Stephen Boesch <ja...@gmail.com>.
Thanks Reynold, Ted Yu did mention offline and I put in a jira already.
Another small concern: there appears to be no exception handling from the
creation of the prepared statement (line 74) through to the executeQuery
(line 86). In case of error/exception it would seem to be leaking
connections (/statements). If that were the case then I would include a
small patch for the exception trapping in that section of code as well.
BTW I was looking at this code for another reason, not intending to be a
bother ;)
2014-08-05 13:03 GMT-07:00 Reynold Xin <rx...@databricks.com>:
> I'm pretty sure it is an oversight. Would you like to submit a pull
> request to fix that?
>
>
>
> On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com> wrote:
>
>> Within its compute.close method, the JdbcRDD class has this interesting
>> logic for closing jdbc connection:
>>
>>
>> try {
>> if (null != conn && ! stmt.isClosed()) conn.close()
>> logInfo("closed connection")
>> } catch {
>> case e: Exception => logWarning("Exception closing connection", e)
>> }
>>
>> Notice that the second check is on stmt having been closed - not on the
>> connection.
>>
>> I would wager this were not a simple oversight and there were some
>> motivation for this logic- curious if anyone would be able to shed some
>> light? My particular interest is that I have written custom ORM's in
>> jdbc
>> since late 90's and never did it this way.
>>
>
>
Re: Tiny curiosity question on closing the jdbc connection
Posted by Reynold Xin <rx...@databricks.com>.
I'm pretty sure it is an oversight. Would you like to submit a pull request
to fix that?
On Tue, Aug 5, 2014 at 12:14 PM, Stephen Boesch <ja...@gmail.com> wrote:
> Within its compute.close method, the JdbcRDD class has this interesting
> logic for closing jdbc connection:
>
>
> try {
> if (null != conn && ! stmt.isClosed()) conn.close()
> logInfo("closed connection")
> } catch {
> case e: Exception => logWarning("Exception closing connection", e)
> }
>
> Notice that the second check is on stmt having been closed - not on the
> connection.
>
> I would wager this were not a simple oversight and there were some
> motivation for this logic- curious if anyone would be able to shed some
> light? My particular interest is that I have written custom ORM's in jdbc
> since late 90's and never did it this way.
>