JAVA-1917: Add ability to set node on statement#1086
Conversation
olim7t
left a comment
There was a problem hiding this comment.
Looks good, see comments inline.
- an additional one: I think
BoundStatementBuilder(BoundStatement)should consider the template statement's node like the other builders.
| - [improvement] JAVA-1925: Rename context getters | ||
| - [improvement] JAVA-1544: Check API compatibility with Revapi | ||
| - [new feature] JAVA-1900: Add support for virtual tables | ||
| - [new feature] JAVA-1917: Add ability to set node on statement |
There was a problem hiding this comment.
Nit: could you add the ticket at the top of the changelog?
I changed this in 4.x because it feels more natural to put more recent tickets first, since versions are ordered that way. It's also how it's done in the Cassandra project.
Maybe we should adopt this convention for the next 3.x versions too for consistency.
| T setNode(@Nullable Node node); | ||
|
|
||
| @Nullable | ||
| Node getNode(); |
There was a problem hiding this comment.
This is a pretty generic feature and it might apply to other types of requests than CQL, so I think Request would be a better place for this method (setNode can stay here, since Request only defines its properties, not how they are set in subtypes).
The only other implementation is PrepareRequest, it should never be targeted so getNode will return null.
Could you also add javadocs? I think the ones from the 3.x PR can almost be copied as-is (the part about not propagating to prepared statements would be better in the javadocs of CqlSession#prepare(SimpleStatement), which already detail all other fields).
| serialConsistencyLevel, | ||
| timeout); | ||
| timeout, | ||
| null); |
There was a problem hiding this comment.
The last argument should be node (inherited from the parent builder) instead of null.
| return self; | ||
| } | ||
|
|
||
| public T withNode(@Nullable Node node) { |
There was a problem hiding this comment.
Could you link to the setter's javadocs like other methods?
| if (this.statement.getNode() != null) { | ||
| this.queryPlan = new ConcurrentLinkedQueue<Node>(); | ||
| this.queryPlan.add(this.statement.getNode()); | ||
|
|
There was a problem hiding this comment.
Since JAVA-1883 there is a dedicated queue implementation for query plans, we can also use it here:
this.queryPlan = new QueryPlan(this.statement.getNode());| // when statement is executed | ||
| ResultSet result = sessionRule.session().execute(statement); | ||
|
|
||
| Node coordinator = result.getExecutionInfo().getCoordinator(); |
| } | ||
|
|
||
| @Test | ||
| public void should_use_host_on_statement() { |
There was a problem hiding this comment.
Nit: 4.x uses the terminology "node" exclusively (I did that to avoid any ambiguity with InetSocketAddress.getHostName, with CASSANDRA-7544 there can now be multiple nodes on the same host).
| Statement statement = SimpleStatement.newInstance(query).setNode(node1); | ||
| // when statement is executed an error should be raised. | ||
| try { | ||
| ResultSet result = sessionRule.session().execute(statement); |
There was a problem hiding this comment.
No need to store the result in a variable here.
| public void should_fail_if_host_is_not_connected() { | ||
| // given a statement with host explicitly set that for which we have no active pool. | ||
| simulacron.cluster().node(4).close(); | ||
| ; |
There was a problem hiding this comment.
Unnecessary semicolumn here and at line 113.
| // when statement is executed | ||
| sessionRule.session().execute(statement); | ||
| fail("Query should have failed"); | ||
| } catch (AllNodesFailedException e) { |
There was a problem hiding this comment.
More precisely, it's a NoNodeAvailableException (a subclass).
Which IMO is also more clear for the end user: "no node was available" and you targeted a node => the targeted node was not available.
Small formatting tweak, Revapi update.
Updating IT to avoid timing issues in jenkins.
Timing fix for IT
As an interface, Statement should not make any assumption on the returned instance. In fact, the default implementation is immutable and the result is a copy.
Moving Node stop to before, to prevent timing issues.
Add ability to setNode on statement, this buypasses the current query plan and creates a new one that contains only the targeted node.