search

puniverse / comsat

Fibers and actors for web development
docs.paralleluniverse.co/comsat
Other
598 stars 103 forks source link

Missing suspendables in comsat-jdbi #84

Open victornoel opened 7 years ago

victornoel commented 7 years ago

Hi,

Apparently the following suspendables is missing in comsat-jdbi:

org.skife.jdbi.v2.sqlobject.SqlObject$3.intercept

Or actually, I suspect that the following suspendables:

org.skife.jdbi.v2.sqlobject.SqlObject$1.intercept
org.skife.jdbi.v2.sqlobject.SqlObject$2.intercept

Should be:

org.skife.jdbi.v2.sqlobject.SqlObject$2.intercept
org.skife.jdbi.v2.sqlobject.SqlObject$3.intercept

As for $1, I don't know if it needs to be marked as suspendable, it is possible, but then it should be:

org.skife.jdbi.v2.sqlobject.SqlObject$1.accept
victornoel commented 7 years ago

I am using jdbi 2.73, but it is the same with 2.72, the version referenced in comsat-jdbi…

victornoel commented 7 years ago

Actually, I think there is a lot more missing…

Some are related to all the SQL Object API way of using jdbi… it's a mess :)

victornoel commented 7 years ago

Here are some others:

org.skife.jdbi.v2.GeneratedKeys.org.skife.jdbi.v2.GeneratedKeys(org.skife.jdbi.v2.tweak.ResultSetMapper,org.skife.jdbi.v2.SQLStatement,java.sql.Statement,org.skife.jdbi.v2.StatementContext,org.skife.jdbi.v2.ContainerFactoryRegistry) (GeneratedKeys.java:57)
org.skife.jdbi.v2.Update$2.munge (Update.java:86)
org.skife.jdbi.v2.util.LongColumnMapper.mapColumn(java.sql.ResultSet,int,org.skife.jdbi.v2.StatementContext) (LongColumnMapper.java:34)
org.skife.jdbi.v2.util.LongColumnMapper.mapColumn (LongColumnMapper.java:22)
org.skife.jdbi.v2.sqlobject.FigureItOutResultSetMapper.map (FigureItOutResultSetMapper.java:35)
pron commented 7 years ago

A PR would be welcome.

victornoel commented 7 years ago

Once I am exactly clear on the problem, I would love to :) For example I don't exactly understand how all of this goes hand in hand with the JdbiClassifier…

See also my post on the mailing list which details the context of these issues: https://groups.google.com/forum/#!topic/comsat-user/j4oIEtxHW0I

pron commented 7 years ago

For example I don't exactly understand how all of this goes hand in hand with the JdbiClassifier…

The classifier is a programmable way of marking suspendable methods. If we're dealing with generated methods (that don't always have the same class name), the classifier would be a good place to add them.

circlespainter commented 7 years ago

@victornoel Could you also add corresponding test cases when you work on the PR? Thanks, expanding test (and use case) coverage is always a great thing.

victornoel commented 7 years ago

@circlespainter sorry, but I'm not sure I will be working on that for now (but if/when I do, I will add tests yes!), because it's too much hassle to make it works and the logs are filled with garbage (I'm not sure why, see the mailing list issue). For the record, I ended up using runBlocking with my own ExecutorService to call jdbi: this ends up doing the same as using the comsat jdbi integration without having to manage the suspendables, since comsat jdbi is based on comsat jdbc which is based on runBlocking.