<enebo>
kares: I fixed an issue on sqlite3 to match AR code more where sqlite3 adapter disconnect! just calls @connection.close but to get the behavior to match AR tests this close() needs to set RubyJdbcConnection field connected to false.
<enebo>
kares: so I just added a disconnect! into sqlite3.close() but this morning I realize that pg adapter Ar definition is also connection.close() but our definition calls connection.disconnect!
<enebo>
ah in fact so does mysql2
<enebo>
So I am wondering if we can just have close call disconnect (in addition to what it does) (well replacing setConnection(null) in close() because disconnect does that
<enebo>
largely the only change to this behavior is that connected = false if you call close()
<enebo>
this retry logic is pretty complex in withConnection so I am game to try this change but I was wondering if you can think of any issues. Perhaps like this hurting extended JDBC behavior
iceman has joined #jruby
<kares>
enebo: personally I do not like that we're moving those kinds of logic to native
<kares>
why not do themin .rb?
<enebo>
kares: I have not moved anything to native
<enebo>
kares: they have always been native for us too
<kares>
the methods yes
<kares>
but native close did not do disconnect
<kares>
that part I mean why not have it in .rb?
<enebo>
it appears to do it in all three adapters in rails 5 though
<kares>
well that I do not care :)
<enebo>
def disconnect! is basically just connection.close in all three
<kares>
is it in base or explicitly?
<kares>
I mean AbstractAdapter
<kares>
if its in .rb let's have it in .rb
<enebo>
each adapter has the exact same impl so I expect at some point they will notice that and migrate it to abstract
<enebo>
but it is not in .rb I don't get your point?
<kares>
I think they do have a reason for that
<kares>
other adapters
<kares>
out there
<kares>
I mean we should copy those methods 3 times in .rb parts
<kares>
not in native
<kares>
I'd like to keep native clean
<kares>
close is just a close in native
<enebo>
I think our close is broken though?
<kares>
in .rb delegating to native close I do not care
<kares>
I do not know you tell me, is it :) ?
<enebo>
We do we not set connect = false
<enebo>
I asked you above? :)
<enebo>
I am confused why it is not set...naively I would think it would be but withConnection logic is not simple
<enebo>
but I really really am hoping to eventually push sqlite3 to point I can remove most of what we have in adapter.rb I am like <20 lines of arjdbc custom ruby
<kares>
but with jndi we will need to be lazy
<kares>
which is desired for users + also due some legacy reconnect behavior which I can not decide if it matters
<enebo>
so if this is a bug then all the adapters can match AR adapters and we won't be breaking anything
<enebo>
legacy in which regards?
<enebo>
java extended behavior I guess
<kares>
auto-reconnecting
<kares>
but that is partially gone
<kares>
so the problem you're seeing is
<kares>
disconnect! not setting connected = false at native right?
<enebo>
so the use case where someone is using more Java AS features and you want connection to try to reuse if it was inadvertently closed?
<enebo>
diconnect! does but close does not
<enebo>
I think both should
<kares>
I do no think close should disconnect in general
<enebo>
but all three main adapters disconnect! impl in Ruby does connection.close
<enebo>
and we have no plans for other adapters other than mssql
<kares>
hmm
<kares>
there's DB2 folks
<kares>
and embedded folks
<kares>
I would rather keep it reasonable ... just in case
<kares>
MS-SQL is pbly the worst thing to support ... ever :)
<enebo>
kares: but I am trying to focus on why we would not just set connection = false in close anyways
<kares>
do not know atm but I feel like there was a reason
<enebo>
I still don't understand what that is supporting (and I admit I don't even understand the difference between close and disconnect! semantically
<kares>
that is confusing
<kares>
it used to be for smt in AR
<enebo>
yeah but I believe both are in AR as well
<enebo>
but maybe it is legacy
<kares>
that is 100%
<kares>
I think close for us might happen as a JNDI conn.close
<enebo>
I can look into that a little bit but I would like to just match in this case because I predict this will make it into abstract sooner or later
<kares>
in that case it would make sense to not disconnect
<kares>
enebo: well I predict it will not :)
<kares>
we can make a bet
<kares>
:)
<enebo>
kares: hahaha
<kares>
since they pbly need to account for other adapters extending base ...
<enebo>
kares: yeah I have no crystal ball
<enebo>
other adapters can override the common behavior though
<kares>
me neither but here's the confusing piece for me - without looking
<kares>
they have AR pool keeping connections around
<kares>
think they used to close conn at end of conn - why would they disconnect
<kares>
I need to check
<enebo>
seems like in ARJDBC JNDI we should just be ignoring close period
<enebo>
I mean lifecycle of that pool is independent of Rails
<kares>
you need to issue a close at the Java connection level
<kares>
otherwise the pool will assume its leaking
<kares>
we can not ignore
<kares>
it is not independent - we expect to be told when request finished
<kares>
end need to delegate a close down to native
<enebo>
kares: but this seems like all adapters working around this one uncommon scenario would almost be better served as an explicit (if (backedByJNDI) { } else { what all other things commonly do })
<kares>
:))
<enebo>
I am wondering what other things are different in a JNDI env?
<kares>
don't like it
<enebo>
is it only close?
<kares>
I rather have it explicitly copied as AR has it in .rb
<kares>
if it moves to base we can adjust just fine
<enebo>
what do you mean has it in .rb
<kares>
but if we force it over to anyone its harder to adapt
<enebo>
I think we are missing each other in this conversation
<kares>
well I feel you're against copying those .rb lines into 3 places?
<kares>
the custom close() method
<kares>
for each of the 3 adapters
<enebo>
We have ruby methods right now calling this but in mysql and pg we changed it to do it "our way" and sqlite3 I will need to override the way it is in the pristine module
<enebo>
I honestly think in cases of both MySql and PostgresQL we could just delete our ruby method and it would just work if we changed close
<enebo>
I would need to look a little closer though to be sure
<kares>
you mean ConnectionManagement in AR-JDBC?
<enebo>
trying to find it just a sec
<enebo>
lib/arjdbc/abstract/connection_management.rb has a disconnect! but that was rdubya trying to make some generic methods. I think if this method is just deleted MySql at least will probably just fall back to AR definition which will call connection.close. I don't know about pg as it is a bit further from pg ruby sources
<enebo>
sqlite3 uses AR source (albeit copied) but I am hoping we will be able to delete that copy.
<enebo>
sqlite3 has <20 lines of custom ruby right now
xardion has quit [Remote host closed the connection]
<kares>
Rob copied those since they were in JdbcAdapter which used to be the base for all
<kares>
its there likely due uniform JNDI support
<enebo>
can I test JNDI support?
<kares>
unless you call disconnect! explicitly you really do not want close to disconnect (at least for JNDI)
<kares>
enebo: well you can if you fix it
<enebo>
kares: another question I have wondered is how many people use JNDI?
<kares>
I'm not into it since with stmnt caching introduced at the Ruby level I am unable to think throught the consequences
<enebo>
kares: you keep griping about this but I don't really have a response to it. I was merely trying to get a Rails 5 release out.
<kares>
well through some work I did regarding JRuby - through the years I would say 50%
<enebo>
50% O_O
<kares>
but I am biased of course I only helped shops that weren't doing simple stuff but 'heavy' Java integration
<enebo>
ok well that is amazing. I should definitely look at it then
<kares>
well its not that easy - to think through everything
<kares>
ideally would really need some PG shop with TC to dive into with some real-world code
<kares>
I am not sure how much flexibility is desired
<kares>
e.g. to turn off the ruby level statement caching etc.
<enebo>
kares: I think lack of knowing how all paths work through a single path is very difficult to support without large amounts of automated testing
<kares>
enebo: okay but we already did it in 1.3
<kares>
wouth automated testing :)
<enebo>
this close example alone is tough if you look at withConnection I think having a JNDI aware one and a non-JNDI one would probably be two much simpler methods
<kares>
its working just fine even with obscure hacks
xardion has joined #jruby
<kares>
withConn is fine
<kares>
its not that hard actually
<enebo>
kares: unless you have to understand it
<kares>
you just cshould not disconnect on close :)
<enebo>
I find it very hard to understand
<kares>
have you used JNDI?
<enebo>
except for non-jndi where it is a semantic
<enebo>
yes
<enebo>
many many years ago :P
<enebo>
like before JRuby
<kares>
:)
<kares>
yeah in Java its simpler
<kares>
since everyone thinks it through
<kares>
here the problem is Rails is ignorant
<kares>
assumes its own pooling
<enebo>
so I see two paths there and you see one extra conditional for non-reconnecting path
<kares>
which actually historically Java deployments have worked around
<kares>
since it used to be a bottleneck
<kares>
so JNDI was legit for performance
<enebo>
yeah I can imagine it helps with general resource management across apps in the same app server too
<enebo>
although the app server deployed world is changing to more isolation via containers
<kares>
yep but with a DB restart I would still trust JNDI over AR's builtin pool
<kares>
I mean a Java pool compared to AR's
<enebo>
kares: so we spent like 15 minutes on this and this line in close would be fine: if (!isJndiConfig()) disconnect(context);
<enebo>
To me it would also be a place to put a well-placed comment explaining that Rails expects connections to not try and re-open whereas JNDI expects the opportunity to retry
<kares>
well since I am not looking into JNDI atm (maybe eventually at some point) - go for it
<kares>
but I am pretty sure it will change
<kares>
since such check wasn't needed before ...
<enebo>
kares: I will only add that line with a nice comment and then delete Sqlite3 definition of close
<kares>
okay
<enebo>
kares: I won't even change our base.rb
<enebo>
although changing it would match rails closer sourcewise
<kares>
well if you can do it in .rb instead of .java I'mall for it :)
<kares>
but I do not care since its just talk from me at this point ...
shellac has quit [Max SendQ exceeded]
<enebo>
well my .rb change would be from .disconnect! to .close
<enebo>
to match AR sources
<kares>
if you run into folks planning 5.x with JNDI send them over :)
shellac has joined #jruby
<enebo>
kares: yeah of course but I would love to find time to see how to try jndi simply
<kares>
its setup in tests
<kares>
but to try it out you really should deploy smt to e.g. Tomcat
<kares>
I mean those tests are really naive - testing parts in isolation
<enebo>
kares: yeah I half wonder if we can use tb somehow
<kares>
although if those parts do not work its pbly not worth deploying ...
<kares>
well TB is a best compared to TC
<kares>
+ its dead, right?
<enebo>
kares: well it still works and we routinely test it for releases