-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Reorganized Async & Sync OperationHelpers #1169
Conversation
Normalized some naming conventions (mainly async oddities) Removed any unused code JAVA-5087
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.
This is prep for the CSOT work. Mainly moved code around using IntelliJ refactoring.
Removed a couple of unused methods and made private those that were only internal to the helpers.
return cursorDocumentToQueryResult(result.getDocument(CURSOR), description.getServerAddress()); | ||
} | ||
|
||
private CommandReadTransformer<BsonDocument, AggregateResponseBatchCursor<T>> transformer() { | ||
private CommandReadTransformer<BsonDocument, QueryBatchCursor<T>> transformer() { |
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.
Used the implementation rather than the interface name (which is the norm across the code).
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.
Ack
@@ -211,7 +211,7 @@ private void resumeableOperation(final AsyncBlock asyncBlock, final SingleResult | |||
|
|||
private void retryOperation(final AsyncBlock asyncBlock, final SingleResultCallback<List<T>> callback, | |||
final boolean tryNext) { | |||
withAsyncReadConnection(binding, (source, t) -> { | |||
withAsyncReadConnectionSource(binding, (source, t) -> { |
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.
Renamed the method (it doesn't return a connection but a source), bringing it inline with the sync naming.
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.
Let's always have "Async" as a suffix, which will make finding any corresponding async method a bit more mechanical? (I did this elsewhere, and it required a wording change in the method to make things grammatical, but I think withReadConnectionAsync
works)
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.
Unfortunately, this is an existing convention with the with
methods - however, they do make sense as withAsyncConnection
provides an AsyncConnection
.
@@ -425,7 +425,7 @@ public void onResult(@Nullable final Void result, @Nullable final Throwable t) { | |||
finalCallback.onResult(null, null); | |||
} else { | |||
executeCommandAsync(binding, databaseName, nextCommandFunction.get(), | |||
connection, writeConcernErrorWriteTransformer(), this); | |||
connection, writeConcernErrorTransformerAsync(), this); |
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.
Renamed the method - bringing it inline with the sync name of its converter and using the Async suffix convention.
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.
Ack
@@ -470,7 +471,7 @@ private boolean isAwaitData() { | |||
return cursorType == CursorType.TailableAwait; | |||
} | |||
|
|||
private CommandReadTransformer<BsonDocument, AggregateResponseBatchCursor<T>> transformer() { | |||
private CommandReadTransformer<BsonDocument, QueryBatchCursor<T>> transformer() { |
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.
Using the implementation name rather than the interface - bringing it inline with the rest of the code base. Same as done in the AggregateOperationImpl
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.
Ack
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.
LGTM, just a naming suggestion. Changes are all simple refactoring:
- method renames
- moving async helper methods to separate helper classes
- removing dead code
(So I have not confirmed, for example, that all moved method contents are the same.)
Normalized some naming conventions (mainly async oddities)
Removed any unused code
JAVA-5087