Align driver with latest revision of ExecuteQuery ADR
#1377
Conversation
Rename `QueryTask` occurrences with `ExecuteQuery` Rename default `ExecuteQuery`'s default `BookmarkManager` getter Make default `ExecuteQuery`'s default `BookmarkManager` unchangeable
fbiville
force-pushed
the
execute-query-adr-follow-up
branch
from
3789a41 to
c25692e
Compare
There was a problem hiding this comment.
I had a catch-up with @AndyHeap-NeoTech today, who is working on the ADR amendment.
Following that, QueryTask and QueryConfig should remain as the are.
BookmarkManager accessor naming should include default word, but I think it will remain configurable upon driver creation (ADR update pending).
| * | ||
| * @return bookmark manager, must not be {@code null} | ||
| * @since 5.6 | ||
| */ | ||
| @Experimental | ||
| BookmarkManager queryTaskBookmarkManager(); | ||
| BookmarkManager defaultExecuteQueryBookmarkManager(); |
There was a problem hiding this comment.
Do we really want that on the drivers level?
Simple API should be simple. There's little value here for the users we are aiming for. If we don't need it, we should not introduce it.
If it is requested, we can still add.
There was a problem hiding this comment.
(To bring context) As far as I remember, this was introduced to allow access to default BookmarkManager to make it possible to use the same instance in SessionConfig, where it must be set explicitly to opt-in. If we still want this to be possible, there should be some way of doing it. I am happy to explore other implementation options.
There was a problem hiding this comment.
The thinking behind having this method was to allow the user to causally chain with uses of the session API, where a bookmark manager can be supplied on session creation.
There was a problem hiding this comment.
Should have refreshed the page...Dmitriy beat me to it.
Rename
QueryTaskoccurrences withExecuteQueryRename default
ExecuteQuery's defaultBookmarkManagergetterMake default
ExecuteQuery's defaultBookmarkManagerunchangeable