ENG-10968 run-everywhere #3843
ENG-10968 run-everywhere #3843
Conversation
|
Can you make the API look like https://issues.voltdb.com/browse/ENG-10296. That has the latest feedback from Andrew. I'm sorry that didn't make it into the ticket https://issues.voltdb.com/browse/ENG-10968 |
| @@ -422,4 +422,36 @@ public boolean updateClasses(ProcedureCallback callback, | |||
| */ | |||
| public VoltBulkLoader getNewBulkLoader(String tableName, int maxBatchSize, boolean upsert, BulkLoaderFailureCallBack blfcb) throws Exception; | |||
| public VoltBulkLoader getNewBulkLoader(String tableName, int maxBatchSize, BulkLoaderFailureCallBack blfcb) throws Exception; | |||
|
|
|||
| /** | |||
| * <p>Synchronously invoke a procedure. Blocks until a result is available. A {@link ProcCallException} | |||
There was a problem hiding this comment.
This doc isn't specific to the new functionality yet?
|
We need test cases of exercising the new API using different parameter type stored procedures. |
| /** | ||
| * <p>Synchronously invoke a single partition procedure. The call will iterate through all the partitions and | ||
| * execute the stored procedure one partition at a time, and return response for each partition. Blocks until a result is available. | ||
| * A {@link ProcCallException} is thrown if the response is anything other then success.</p> |
There was a problem hiding this comment.
This is a really good, unanswered design question. If we have partial failure, should it throw or have some responses returned be failed.
I kinda like the idea that it throws, but then how do you understand what failed and what succeeded? Needs thought.
| return (private_executeAllPartitionProcedure(callback, procedureName, params) != null); | ||
| } | ||
|
|
||
| private ClientResponseWithPartitionKey[] private_executeAllPartitionProcedure(AllPartitionProcedureCallback callback, |
There was a problem hiding this comment.
You don't need a private method here. You can literally call the async version unchanged from the sync version. Just pass in a custom callback that looks like an all-partition version of SyncCallback and block on it.
| * @throws IOException if there is a Java network or connection problem. | ||
| */ | ||
| private void setPartitions() throws IOException, NoConnectionsException, ProcCallException { | ||
| if(m_partitionIntegerKeys.isEmpty()){ |
There was a problem hiding this comment.
It's better to return early for non-empty case and save one indentation, like:
if(! m_partitionIntegerKeys.isEmpty()) {
return;
}
long time = System.currentTimeMillis() - m_lastPartitionKeyFetched;
...
Also, we should have consistent space, like "if (...) {".
There was a problem hiding this comment.
Probably, we can just assert empty collection here. We have one caller knowing to update the partition keys.
assert(m_partitionIntegerKeys.isEmpty());
Maybe rename the function as "refreshPartitionKeys".
| cb.clientCallback(null); | ||
| } | ||
| } | ||
| } catch (ProcCallException | NoConnectionsException pe){ |
There was a problem hiding this comment.
Do not need to catch ProcCallException | NoConnectionsException when you do the same thing for Exception. You can simply remove this catch block.
| try{ | ||
| if(callback instanceof SyncAllPartitionProcedureCallback){ | ||
| try { | ||
| if (callback instanceof OnePartitionProcedureCallback) { |
There was a problem hiding this comment.
Sorry if I do not make it clear.
Keep "SyncAllPartitionProcedureCallback", but rename "PartitionProcedureCallback" as "OnePartitionProcedureCallback".
| } | ||
| Client client = getClient(); | ||
| client.callAllPartitionProcedure(new CallBack(STRING_PARTITION_EXPECTED_COUNTS), "PartitionStringTestProc"); | ||
| Thread.sleep(2000); |
There was a problem hiding this comment.
The Async test version does not necessary validate the result. It could be the "callAllPartitionProcedure" never return anything. We want to ensure the "clientCallback" has been executed.
|
The rest looks good to me. We can ask Andrew to review the Java client doc later. |
| @@ -3,16 +3,14 @@ CREATE TABLE TABLE_INT_PARTITION | |||
| id bigint NOT NULL, | |||
There was a problem hiding this comment.
Can you also fix the indentation ?
| public boolean callAllPartitionProcedure(AllPartitionProcedureCallback callback, String procedureName, | ||
| Object... params) throws IOException, NoConnectionsException, ProcCallException { | ||
| if (callback == null) { | ||
| throw new IOException("AllPartitionProcedureCallback can not be null"); |
There was a problem hiding this comment.
Should this be an IOException or an IllegalArgumentException?
Added a sync and an async method which go through partitions for the stored proc to be executed.