IGNITE-20295 Sql. Get rid of server-side session management#2811
IGNITE-20295 Sql. Get rid of server-side session management#2811korlov42 merged 8 commits intoapache:mainfrom
Conversation
korlov42
force-pushed
the
ignite-20295
branch
from
4ea826c to
acc745d
Compare
korlov42
force-pushed
the
ignite-20295
branch
from
acc745d to
abd5560
Compare
| return qryProc.querySingleAsync(properties, transactions, (InternalTransaction) transaction, query, args) | ||
| .thenCompose(cursor -> { | ||
| if (!busyLock.enterBusy()) { | ||
| return CompletableFuture.failedFuture(sessionIsClosedException()); |
There was a problem hiding this comment.
Should we close cursor here?
There was a problem hiding this comment.
It seems the same applies to executeAsync.
There was a problem hiding this comment.
totally agree, should check it myself (
fixed
...les/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/ChainedIterator.java
Outdated
Show resolved
Hide resolved
- close query cursor if batch execution was interrupted - replace ChainedIterator with CollectionUtils#concat
- close query cursor if execution was interrupted: single statement
| } | ||
|
|
||
| @TestOnly | ||
| List<AsyncSqlCursor<?>> openedCursors() { |
There was a problem hiding this comment.
t seems that it is enough for this method to return int 😎
There was a problem hiding this comment.
if it's matter of taste (you don't have strong reason to hide cursors), then I would prefer to leave it as is
| /** | ||
| * Exception that is thrown by {@link PropertiesHolder} if given property | ||
| * is not fount in the holder. | ||
| * Exception that is thrown by {@link SqlProperties} if given property is not fount. |
There was a problem hiding this comment.
| * Exception that is thrown by {@link SqlProperties} if given property is not fount. | |
| * Exception that is thrown by {@link SqlProperties} if given property is not found. |
| * @param secondary A secondary object. | ||
| * @return A chained properties object. | ||
| */ | ||
| public static SqlProperties chain(SqlProperties primary, SqlProperties secondary) { |
There was a problem hiding this comment.
I didn't find a test for this method 😎
There was a problem hiding this comment.
tests have been added
| when(queryProcessor.createSession(any())) | ||
| .thenAnswer(ignored -> new SessionId(UUID.randomUUID())); | ||
|
|
||
| return new IgniteSqlImpl("test", queryProcessor, mock(IgniteTransactions.class)); |
There was a problem hiding this comment.
This test now runs successfully without mocks 😎
There was a problem hiding this comment.
although it works now if we pass null instead of mocks, such an approach doesn't feel safe. Neither QueryProcessor nor IgniteTransactions are marked as @Nullable. I don't want this test to fail if someone will decide to add additional assertions to ensure null-safety
| * @param properties User query properties. See {@link QueryProperty} for available properties. | ||
| * @param transactions Transactions facade. | ||
| * @param transaction A transaction to use for query execution. If null, an implicit transaction | ||
| * will be started by provided transactional facade. |
There was a problem hiding this comment.
| * will be started by provided transactional facade. | |
| * will be started by provided transactions facade. |
🤔 🤔
- fix javadocs - add tests for SqlPropertiesHelper#chain
3 participants
https://issues.apache.org/jira/browse/IGNITE-20295
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes