Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocking DBClient Part 1 #6991

Closed
Tracked by #6683
romain-grecourt opened this issue Jun 13, 2023 · 9 comments · Fixed by #7096
Closed
Tracked by #6683

Blocking DBClient Part 1 #6991

romain-grecourt opened this issue Jun 13, 2023 · 9 comments · Fixed by #7096
Assignees
Labels
4.x Version 4.x DB client Helidon DB Client Níma Helidon Níma
Milestone

Comments

@romain-grecourt
Copy link
Contributor

romain-grecourt commented Jun 13, 2023

Here is the initial proposal for the update of DBClient to a blocking style API.


DbExecute.

public interface DbExecute extends AutoCloseable {
    Stream<DbRow> query(String statement, Object... parameters);
    Optional<DbRow> get(String statement, Object... parameters);
    long insert(String statement, Object... parameters);
    long update(String statement, Object... parameters);
    long delete(String statementName, Object... parameters);
    long dml(String statement, Object... parameters);
}

AutoClosable

Note the use of AutoClosable.

In the blocking style, onComplete is achieved by the next statement.

List<String> names = exec.query("SELECT * FROM names").toList();
System.out.println("done");

onError is achieved by a try-catch block:

try {
    List<String> names = exec.query("SELECT * FROM names").toList();
    System.out.println("done");
} catch (DbClientException dce) {
    // ugh-oh
}

DbExecute should work like a session, where the object can be used multiple times to perform database operations.
If DbExecute is designed as single-use, we should introduce a DbSession class that implements AutoClosable.

The try-with-resources block represents code that uses a session:

try (DbExecute exec = dbClient.execute()) {
  // use exec
}

Transaction

DbTransaction extends DbExecute and defines a contract where commit() MUST be called.
When the auto close() is called, all non-committed and non rollbacked transactions are rollbacked.

public interface DbTransaction extends DbExecute {
    void commit();
    void rollback();
}
try (DbTransaction tx = dbClient.transaction()) {
    long count = tx.delete("DELETE FROM pokemons WHERE name = 'foo'");
    tx.commit();
    return count;
}

Stream.close

Use Stream close handler as an API to close the related ResultSet (for JDBC). However since Stream does not require try-with-resources, hook the closing of the stream as part of the enclosing AutoClosable (I.e. DbExecute).

This means, if the user uses a Stream produced by DbStatementQuery outside of a try-with-resources, then the stream is unusable.

The creation of the Stream<DbRow> can be done like this:

Stream<DbRow> stream = StreamSupport.stream(new AbstractSpliterator<DbRow>(Long.MAX_VALUE, Spliterator.ORDERED) {
    @Override
    public boolean tryAdvance(Consumer<? super DbRow> action) {
        try {
            if (!rs.next()) {
                return false;
            }
            DbRow dbRow = createDbRow(rs, metadata, dbMapperManager, mapperManager);
            action.accept(dbRow);
            return true;
        } catch (SQLException ex) {
            throw new DbClientException(ex.getMessage(), ex);
        }
    }
}, false).onClose(() -> {
    try {
        rs.close();
    } catch (SQLException e) {
        throw new DbClientException("Failed to close result-set", e);
    }
});

Boxed Long

Remove the result type parameter from DbStatement in order to avoid forcing a boxed Long as a return type.
This means that the execute method has to be defined in all sub interfaces of DbStatement:

public interface DbStatementQuery extends DbStatement<DbStatementQuery> {
    Stream<DbRow> execute();
}
public interface DbStatementGet extends DbStatement<DbStatementGet> {
    Optional<DbRow> execute();
}
public interface DbStatementDml extends DbStatement<DbStatementDml> {
    long execute();
}

Interceptor

Update the interceptor mechanism to be blocking friendly similar to WebClientService:

public interface DbClientService {

    Object handle(Chain chain, DbClientServiceContext context);

    interface Chain {
        Object proceed(DbClientServiceContext context);
    }
}

MongoDB

  • Drop transaction support for MongoDB, throw an UnsupportException in transaction()
  • Use the blocking API of Mongo directly

Misc

  • Inline dbclient/common in the main module
  • Remove all ExecutorService related code
  • Remove all mentions of reactive, non-blocking
@romain-grecourt romain-grecourt added DB client Helidon DB Client 4.x Version 4.x Níma Helidon Níma labels Jun 13, 2023
@romain-grecourt romain-grecourt added this to the 4.0.0-M1 milestone Jun 13, 2023
@trentjeff
Copy link
Member

What if the user really wants to use ExecutorService? For instance, if the user wants to have a timeout before canceling the query?

@romain-grecourt
Copy link
Contributor Author

Unlike the reactive DBClient, the blocking DBCLient API does not need an executor service, it runs on whatever thread you are running on.

@Tomas-Kraus
Copy link
Member

Tomas-Kraus commented Jun 14, 2023

Timeout on statement execution makes sense in some corner cases. But I'd better examine JDBC API options and few other things before starting any threads. Also 1st step is to adopt existing reactive API and make 1:1 copy of blocking API/implementation.
I'll try to preserve as much as possible from existing code patterns, but as Romain already mentioned, all reactive support has to be removed.
Additional things may be added in follow up issues. But let's to the minimal required changes to fulfill this goal first.

Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 23, 2023
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 23, 2023
…AutoCloseable

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 26, 2023
…fixed and simple DML tests are now passing.

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
@Tomas-Kraus Tomas-Kraus linked a pull request Jun 26, 2023 that will close this issue
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 27, 2023
…gration tests are passing

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 28, 2023
…n and execution

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 28, 2023
…pe from DbStatement. Code cleanup.

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 28, 2023
…pe from DbStatement. Code cleanup.

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
@Tomas-Kraus
Copy link
Member

Tomas-Kraus commented Jun 28, 2023

@romain-grecourt Looks like the only thing that needs close() is Stream<DbRow>.
DML/Get conenction/statement/resultset is closed before result (long or Optional) is returned from execute method. Those are single shot results so I can do that.
Query result is Stream<DbRow>. So execute opens conenction/statement/resultset and passes them to onClose method of the stream. Stream reads ResultSet on the fly so everything must be opened during Stream processing phase.

I'll added javadoc comments to DbStatementQuery so hope users will at least read it.

Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 29, 2023
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
@Tomas-Kraus
Copy link
Member

Tomas-Kraus commented Jun 30, 2023

Progress:

  • Adopt API
    • done except ClientService (interceptors)
  • JDBC
  • ClientService (interceptors)
  • MongoDB - not started yet
  • Health check - not started yet
  • Metrics - not started yet
  • JsonP - not started yet
  • Tracing - not started yet

Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 30, 2023
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 30, 2023
…er tests.

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 30, 2023
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jun 30, 2023
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
@romain-grecourt
Copy link
Contributor Author

romain-grecourt commented Jun 30, 2023

Update on closing.

Making DbExecute extend AutoCloseable forces everything to be done with a try-with-resources when in reality only query statements need closing. Single execution statements can be closed right away.

Using Stream.close() doesn't warn about the need to close like other AutoCloseable, thus it makes the API a bit unsafe if calling .close() is a requirement.

We can mitigate the issue by automatically closing on all terminal operations in the stream. This means the implementation has to use a stream delegate that intercepts all terminal operations in the stream.

We can also have a fail-safe where the Spliterator that powers the stream can ensure that the connection and result set are closed when the result set has nothing left.

This is a win-win, as we have less boilerplate and less requirement in order to properly use the API.

However, it is still possible to mis-use the stream API and consume items without using a terminal operation ; in this case the users must call .close(). This seems acceptable.

@zimmi
Copy link

zimmi commented Jul 1, 2023

Hi! Unsurprisingly, there is a lot of prior art when it comes to JDBC wrappers. I quite liked https://github.com/zsoltherpai/fluent-jdbc , which sports a similar API to what is worked on here. Maybe it can serve for inspiration? Thank you for working on this! 👍

Edit: I'd also like to mention https://github.com/OpenGamma/ElSql which I've used in the past for externalizing SQL. Having those two kinds of libraries together provides enough abstraction layer for many apps. Maybe something like this can be considered as an extension to the current approach?

Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jul 3, 2023
…ial implementation in JDBC

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
@romain-grecourt
Copy link
Contributor Author

Edit: I'd also like to mention https://github.com/OpenGamma/ElSql which I've used in the past for externalizing SQL. Having those two kinds of libraries together provides enough abstraction layer for many apps. Maybe something like this can be considered as an extension to the current approach?

Currently queries can be defined externally using Helidon Config, we can consider an SPI to have an extension that integrates something like ElSql.

Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jul 7, 2023
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
Tomas-Kraus added a commit to Tomas-Kraus/helidon that referenced this issue Jul 7, 2023
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
tomas-langer pushed a commit to Tomas-Kraus/helidon that referenced this issue Jul 14, 2023
Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
tomas-langer pushed a commit to Tomas-Kraus/helidon that referenced this issue Jul 14, 2023
…ial implementation in JDBC

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
tomas-langer pushed a commit to Tomas-Kraus/helidon that referenced this issue Jul 14, 2023
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
tomas-langer pushed a commit to Tomas-Kraus/helidon that referenced this issue Jul 14, 2023
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
barchetta pushed a commit that referenced this issue Jul 14, 2023
* Issue #6991 - Blocking DB Client: API, JDBC and Health

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>

* Issue #6991 - Blocking DB Client: Interceptors API and initial implementation in JDBC

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>

* Issue #6991 - Blocking DB Client: Tracing support

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>

* Issue #6991 - Blocking DB Client: Common metrics module

Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>

* - Fold dbclient-common into dbclient
- Implement dbclient-mongodb
- Re-work indexed/named parameters
- Convergence between DbClientContext and DbClientExecuteContext
- Generalized intercepted executions
- Removed package-private factory methods in favor of constructors
- Added javadocs
- Copyright and checkstyle fixes

* add missing newline

* - Fix LRA
- Fix examples/employee-app
- Fix examples/dbclient

* - Fix spotbugs errors
- Add static factory to MongoDbClientBuilder

* - Add metrics-jdbc
- Add jsonp
- Remove change in reactive/dbclient/jdbc

* Add dbclient tests

* - Rename io.helidon.dbclient.DbMapperProvider to io.helidon.dbclient.DbMapperProviderImpl to avoid confusion
- Fix module-info to have uses io.helidon.dbclient.spi.DbMapperProvider
- Fix connection close for transaction statements

* integration tests (app)

* - Fix copyright years
- Consistent naming (DB Client -> Database Client)
- Minize occurrences of the term "Pokemon" and use {@code Pokemon} when required.

* Testing work.

* Update maven-javadoc-plugin to 3.5.0 to get passed https://issues.apache.org/jira/browse/MJAVADOC-677

* Rebased on main, fixes to dbclient metrics

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>

---------

Signed-off-by: Tomáš Kraus <tomas.kraus@oracle.com>
Signed-off-by: Tomas Kraus <tomas.kraus@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Co-authored-by: Romain Grecourt <romain.grecourt@oracle.com>
Co-authored-by: Tomas Langer <tomas.langer@oracle.com>
@barchetta barchetta changed the title Blocking DBClient Blocking DBClient Part 1 Jul 14, 2023
@barchetta
Copy link
Member

This issue tracks the work done for 4.0.0-M1. For follow-up work see issue #7187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x DB client Helidon DB Client Níma Helidon Níma
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants