-
Notifications
You must be signed in to change notification settings - Fork 24
Async refactor #175
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
base: main
Are you sure you want to change the base?
Async refactor #175
Conversation
This is the initial commit for support of Async Java APIs. This commit address some of the major code changes needed for Async, which are listed below. This commit does not contain code changes for new Async NoSQLHandle but provides scaffolding for the same. - Java version is updated to 11 from 8 - Client#execute, which is the core of the request processing, is now returns CompletableFuture<Result>. This method is changed to non-blocking with retries and error handling - HttpClient#runRequest returns CompletableFuture<FullHttpResponse>. HttpClient now abstract out the Channel acquisition and submitting the request along with timeout. Consumers of the HttpClient need not worry about channel acquisition and release. The new signature of runRequest method is `CompletableFuture<FullHttpResponse> runRequest(HttpRequest request, int timeoutMs)` - AuthorizationProvider API is updated to introduce async versions of getAuthorizationString and setRequiredHeaders - NoSQLHandleImpl sync methods are updated to wait on CompletableFuture<Result> - StoreAccessTokenProvider and SignatureProvider are refactored into non-blocking classes Testing: All existing unit tests are passing
executeWithRetry method is very big and challenging to comprehend. Split this into multiple methods to handle individual stages like, check for rate limit, get auth token from auth provider, serialize the request, send request to server, process response, and so on.
- Incorporate changes suggested by George
o. Limit line width to 80 characters
o. Keep core part of SignatureProvider i.e.
SignatureProvider#getSignatureDetailsInternal same as existing blocking code
and wrap it with CompletableFuture.supplyAsync
o. Keep core part of StoreAccessTokenProvider i.e. bootstraplogin same as existing
blocking code and wrap it with CompletableFuture.supplyAsync
o. Implement flushCache method for StoreAccessTokenProvider. In the current
implementation, flushCache is not implemented for the StoreAccessTokenProvider,
which can result in bootstrap login to never contact the server after the
initial acquisition of the auth token
o. Revert HttpRequestUtil to a blocking version as this code always executes in blocking mode
- Error handling enhancement for client execute
o. In the current implementation error handling is a very big piece of code with
lots of catch blocks with a lot of repeated code.
o. Introduced a strategy pattern where a HashMap<Class<? extends Throwable>,
ErrorHandler> is used to maintain a list of possible exceptions encountered by
the system and error handler for each exception. In this way, we can keep the
core error handling and retry code to be simple and delegate actual error to
handlers.
o. Please see below for more details
Files:
M driver/src/main/java/oracle/nosql/driver/DefaultRetryHandler.java
M examples/src/main/java/Common.java
M driver/src/main/java/oracle/nosql/driver/httpclient/ConnectionPool.java
M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java
- Limit line width to 80
M driver/src/main/java/oracle/nosql/driver/http/Client.java
- Use core number of threads for task executor
- Introduced a ErrorHandler interface to handle individual errors
- Added a condition to check overall timeout before retrying a request
- Split handleError() method to multiple error handlers
- Added findErrorHandler() method which finds the specific handler for given error
- Replaced synchronized blocks with ReentrantLock
M driver/src/main/java/oracle/nosql/driver/http/NoSQLHandleImpl.java
- Replaced call to CompletableFuture.get() with utility method ConcurrentUtil.awaitFuture
M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClientHandler.java
- When timeout occurs throw java.util.concurrent.TimeoutException instead of Netty ReadTimeoutException
M driver/src/main/java/oracle/nosql/driver/iam/FederationRequestHelper.java
M driver/src/main/java/oracle/nosql/driver/iam/InstanceMetadataHelper.java
M driver/src/main/java/oracle/nosql/driver/iam/OkeWorkloadIdentityProvider.java
- Revert async changes
M driver/src/main/java/oracle/nosql/driver/iam/SignatureProvider.java
- Added async wrapper over core getSignatureDetailsInternal method
- Replaced synchronized blocks with ReentrantLock
M driver/src/main/java/oracle/nosql/driver/kv/StoreAccessTokenProvider.java
- Added async wrapper over core bootstrap login method
- Override flushCache and implement
- Replaced synchronized blocks with ReentrantLock
M driver/src/main/java/oracle/nosql/driver/query/ReceiveIter.java
- Replaced call to CompletableFuture.get() with utility method ConcurrentUtil.awaitFuture
M driver/src/main/java/oracle/nosql/driver/util/ConcurrentUtil.java
- Added a utility method awaitFuture() to wait on future
- Added a utility method unwrapCompletionException()
M driver/src/main/java/oracle/nosql/driver/util/HttpRequestUtil.java
- Reverted async changes
M driver/src/main/java/oracle/nosql/driver/util/LogUtil.java
- Added a utility method getStackTrace() to get stack trace from Throwable
- synchronous keep-alive
o. Keep-alive used to happen on Netty event-loop threads. Introduced a new single thread for keep-alive and also made the keep-alive synchronous
o. Fixed an issue in Connection pool stats collection where ChannelStats is added to the Map even for removal
- Async NoSQL Handle
o. Added new NoSQLHandleAsync interface and corresponding NoSQLHandleAsyncImpl implementation
o. Added a new method NoSQLHandleAsync#queryPaginator, which returns Flow.Publisher<MapValue>
o. queryPaginator is an async version of queryIterable
o. Updated NoSQLHandleImpl to internally use NoSQLHandleAsyncImpl
o. Added static method to create NoSQLHandleAsync in NoSQLHandleFactory
o. Added an async method TableResult#waitForCompletionAsync in TableResult
o. Added an async method SystemResult#waitForCompletionAsync in SystemResult
Files:
A driver/src/main/java/oracle/nosql/driver/NoSQLHandleAsync.java
A driver/src/main/java/oracle/nosql/driver/http/NoSQLHandleAsyncImpl.java
- Async NoSQL handle addition
A driver/src/main/java/oracle/nosql/driver/ops/QueryPublisher.java
- Added a Publisher for query paginator. The Publisher is backpressure aware.
It will contact the server only when there is more demand from
downstream subscriber. The Publisher also closes the QueryRequest internally
M driver/src/main/java/oracle/nosql/driver/NoSQLHandleFactory.java
- Added a static method to create NoSQLHandleAsync
M driver/src/main/java/oracle/nosql/driver/http/Client.java
- Explicitly pass task executor to use
M driver/src/main/java/oracle/nosql/driver/http/NoSQLHandleImpl.java
- Changed to be wrapper around async handle
M driver/src/main/java/oracle/nosql/driver/httpclient/ConnectionPool.java
- Added new single thread pool to run keep-alive in background and reverted async keep-alive
- In updateStats() channel has been added to map even for remove case. Added a check for isAcquire
- Reverted the signature of keepALive interface to boolean from CompletableFuture<Boolean>
M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java
M driver/src/test/java/oracle/nosql/driver/httpclient/ConnectionPoolTest.java
- Reverted the signature of keepALive interface to boolean from CompletableFuture<Boolean>
M driver/src/main/java/oracle/nosql/driver/ops/SystemResult.java
M driver/src/main/java/oracle/nosql/driver/ops/TableResult.java
- Added async version of waitForCompletion method
- Fix review comments by Jin Files: M driver/src/main/java/oracle/nosql/driver/AuthorizationProvider.java M driver/src/main/java/oracle/nosql/driver/NoSQLHandleFactory.java M driver/src/main/java/oracle/nosql/driver/http/Client.java M driver/src/main/java/oracle/nosql/driver/http/NoSQLHandleAsyncImpl.java M driver/src/main/java/oracle/nosql/driver/http/NoSQLHandleImpl.java M driver/src/main/java/oracle/nosql/driver/httpclient/ConnectionPool.java M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java M driver/src/main/java/oracle/nosql/driver/httpclient/RequestState.java M driver/src/main/java/oracle/nosql/driver/httpclient/ResponseHandler.java M driver/src/main/java/oracle/nosql/driver/iam/SignatureProvider.java M driver/src/main/java/oracle/nosql/driver/kv/StoreAccessTokenProvider.java M driver/src/main/java/oracle/nosql/driver/ops/QueryPublisher.java M driver/src/main/java/oracle/nosql/driver/ops/SystemResult.java M driver/src/main/java/oracle/nosql/driver/ops/TableResult.java M driver/src/main/java/oracle/nosql/driver/util/ConcurrentUtil.java M driver/src/main/java/oracle/nosql/driver/util/HttpRequestUtil.java - Fix indentation, limit line width to 80, remove unused imports, add copyrights M driver/src/main/java/oracle/nosql/driver/http/Client.java - Remove duplicate entry of kvRequest.addRetryDelayMs() before retrying the request M driver/src/main/java/oracle/nosql/driver/ops/SystemResult.java M driver/src/main/java/oracle/nosql/driver/ops/TableResult.java M driver/src/main/java/oracle/nosql/driver/RetryHandler.java - Added javadocs for public APIs M driver/src/main/java/oracle/nosql/driver/package-info.java - Added TODO to include doc about Async client
| */ | ||
| void delay(Request request, int numRetries, RetryableException re); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Add Javadoc?
Added Javadoc as suggested by you
| String name = ex.getClass().getName(); | ||
| logFine(logger, "Client execution IOException, name: " + | ||
| name + ", message: " + ex.getMessage()); | ||
| /* Retry only 10 times. We shouldn't be retrying till timeout occurs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: We don't limit the number of retries before, why do we need this now?
IMO, this is a overlook in existing code. It is always better to have upper bound on number of retries. Usually most of the IOExcpetions are like connection refused or UnknownHost which can't be recovered.
| ctx.requestId = String.valueOf(ctx.nextIdSupplier.get()); | ||
| executeWithRetry(ctx) | ||
| .whenComplete((res, e) -> { | ||
| if (e != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Possible duplicate — retryRequest() already calls kvRequest.addRetryDelayMs()
You're correct. Removed this
| handler.channelAcquired(channel); | ||
| } catch (Exception e) {} /* ignore */ | ||
| if (!promise.trySuccess(channel)) { | ||
| release(channel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Should we attempt to re-acquire the channel in this case?
No. promise is already completed and no need to retry here
| promise.tryFailure(t); | ||
| } | ||
| } | ||
| promise.tryFailure(cause); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Should we attempt to re-acquire the channel in this case?
No. This is an error scenario. No need to reacquire
| * </ol> | ||
| * <p> | ||
| * Using the client to send request and get a synchronous response. The | ||
| * Using the client to send request. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: The JavaDoc above might need to be updated.
Updated the Javadoc for new APIs
| * desired here it can be added using a CompositeByteBuf and calls to add | ||
| * content incrementally. | ||
| */ | ||
| /* TODO: this class is no longer used. Remove this once testing is completed */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: this class seems unused?
Yes. Will remove this in future. Added a TODO to remove
| * | ||
| * TODO: examples of both sync and async usage | ||
| */ | ||
| /* TODO: this class is no longer used. Remove this once testing is completed */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: this class seems unused?
Yes. Will remove this in future. Added a TODO to remove
| } | ||
| } while (!state.equals(State.COMPLETE)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Add Java doc?
Added javadoc
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: Add Java doc?
Added javadoc
| * Licensed under the Universal Permissive License v 1.0 as shown at | ||
| * https://oss.oracle.com/licenses/upl/ | ||
| */ | ||
| /* TODO: need add NoSQLHandleAsync? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC: might need add NoSQLHandleAsync?
Added TODO. I'll add later
- Change package version to 6.0.0 - Updated javadocs of NoSQLHandleAsync to mention the CompletionException. When CompletableFuture returned by the async APIs completes exceptionally, the thrown exception is CompletionException. The actual cause is wrapped in CompletionException - Added CHANGELOG.md entry - Added a new smoke test for async APIs Files: M CHANGELOG.md - Added an entry for unreleased 6.0.0 version M README.md M driver/pom.xml M examples/pom.xml M pom.xml M driver/src/main/java/oracle/nosql/driver/SDKVersion.java - Changed version to 6.0.0 M driver/src/main/java/oracle/nosql/driver/NoSQLHandleAsync.java - Updated javadocs for CompletionException M driver/src/test/java/oracle/nosql/driver/BasicAsyncTest.java M driver/src/test/java/oracle/nosql/driver/ProxyTestBase.java - Added new async tests
- Fixed netty bytebuf leak - Fixed NullPointer exception in BasicAsyncTest - Fixed double writing to bytebuf in HttpRequestUtil - Updated javadoc for HttpClient.runRequest Files: M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java - Added javadoc for runRequest - Release HttpResponse in keep-alive path M driver/src/main/java/oracle/nosql/driver/util/HttpRequestUtil.java - Fix double writing to bytebuf M driver/src/test/java/oracle/nosql/driver/BasicAsyncTest.java - Fix NullPointer issue
Connection pooling changes - Added new connection pool `IdleEvictFixedChannelPool` to limit the maximum number of connections. This is necessary in the async world, where a single thread can fire huge number of requests in a very short period of time. Existing unbounded connections put pressure on system resources and also eventually lead to too many files(sockets) open error. To tackle this, two new parameters 'connectionPoolSize' and 'poolMaxPending' are re-introduced. These configs were present earlier and later removed - connectionPoolSize - The maximum number of connections (upper bound) in the connection pool, if this number is reached, further connection acquisition requests are queued up to poolMaxPending - poolMaxPending - The maximum number of pending acquires for the pool. If this exceeds, new connection acquires result in error until connections are released back to the pool - The IdleEvictFixedChannelPool class wraps Netty's FixedChannelPool to track the pool metrics and to evict idle channels from the pool after idle timeout. This class simplifies the existing ConnectionPool implementation by removing keep-alive on minimum number of channels and using Netty's native IdleChannelHandler to remove the idle channels instead of running a dedicated periodic job to modify the internal state of the pool which is not possible for Netty FixedChannelPool - Channel keep-alive task is removed. In the existing implementation keep-alive job periodically runs keep-alive on two idle connections to retain the connections for future requests and there by reducing the latency. This hardly reduces P95 latency and difficult to implement on top of Netty FixedChannelPool. However, Idle time out and evict of channels is still retained. Now, all the idle channels are evicted. - Added unit tests for IdleEvictFixedChannelPool and performance tests for async APIs Files: A driver/src/main/java/oracle/nosql/driver/httpclient/IdleEvictFixedChannelPool.java A driver/src/test/java/oracle/nosql/driver/httpclient/IdleEvictFixedChannelPoolTest.java - New connection pool impl for fixed number of connections - Unit tests M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java - Added a new constructor to take maxConnections and maxPending - Replace ConnectionPool with IdleEvictFixedChannelPool - Removed acquireRetryIntervalMs - connectionPoolMinSize and isMinimalClient are no longer used as keep-alive is unnecessary - Added timeout for channel acquisition from the pool. This timeout is set to request timeout M driver/src/main/java/oracle/nosql/driver/NoSQLHandleConfig.java - Added default value for connectionPoolSize and poolMaxPending - Removed deprecate javadoc for pool size and pending size - Updated javadoc to deprecate min pool size M driver/src/main/java/oracle/nosql/driver/http/Client.java - Pass max connection and pending connection to HttpClient constructor M driver/src/test/java/oracle/nosql/driver/ProxyTestBase.java - Changed default request time for test to 5000 ms. This is the default used by production code A driver/src/test/java/oracle/nosql/driver/PerformanceTest.java - Async PerformanceTest
Connection pooling changes - Updated ConnectionPool to support maxConnections and maxPending. Modified acquire and release flow to honor maxConnections and maxPending connections. Existing keep-alive and idle channel evict should work out of the box. - Removed IdleEvictFixedChannelPool.java as found it difficult to add keep-alive for this implementation - Removed IdleEvictFixedChannelPoolTest.java - Removed unused ResponseHandler.java and RequestState.java - Disable PerformanceTest for unit testing. Jenkins kvserver is set to maxActiveRequests=100 Files: M .gitignore - Added .idea and .oca to ignore list M driver/src/main/java/oracle/nosql/driver/NoSQLHandleConfig.java - Reverted deprecation of minPoolSize M driver/src/main/java/oracle/nosql/driver/httpclient/ConnectionPool.java M driver/src/test/java/oracle/nosql/driver/httpclient/ConnectionPoolTest.java - Added maxConnection and maxPending M driver/src/main/java/oracle/nosql/driver/httpclient/HttpClient.java - Reverted minPool removal M driver/src/test/java/oracle/nosql/driver/PerformanceTest.java - Disabled for unit testing D driver/src/main/java/oracle/nosql/driver/httpclient/IdleEvictFixedChannelPool.java D driver/src/main/java/oracle/nosql/driver/httpclient/RequestState.java D driver/src/main/java/oracle/nosql/driver/httpclient/ResponseHandler.java D driver/src/test/java/oracle/nosql/driver/httpclient/IdleEvictFixedChannelPoolTest.java - Removed unused classes
This is the PR for supporting Async Java APIs.
CompletableFuture and Flow.Publisher based async APIs are introduced. The new async APIs are similar to existing sync blocking APIs but returns CompletableFuture for single item response or Flow.Publisher for multiple item responses(Query in this case). Also, Existing sync APIs are converted to be wrapper around core async APIs and wait for the response. This way, we don't need to maintain separate implementations for both.
Java version is bumped to 11 from 8. So, basically this is a breaking change and we need to update driver version to 6.0.0?
Netty based HttpClient is refactored into return CompletableFuture of Netty HttpResponse.
Please see the commit logs for more details