Allow clients to request the transactor to retry queries

[Sherif A. Nada (sherifnada)]

Liren Tu (@tuliren)

This change introduces the ExecutionContext concept. It allows a client of an ITransactor to configure settings to be applied on the next execution or query. Today, the only such option is whether to execute the query as a transaction, specified by queryAsTransaction() and executeAsTransaction(). However, as the number of options increases (the current PR ultimately aims to add a retry mechanism), it becomes less feasible to add methods that both set the flag and execute the query. ExecutionContext will be used to allow queries that look like this:

transactor.asTransaction()
  .withNumRetries(3)
  .query(myQuery); // can also be .execute()

or

transactor.withNumRetries(3).query(myQuery);

or even

transactor.query(myQuery);

if they would rather use the default options. In the future, if we want to add more options, we can just add a .withNewOption() method to the interface.

This would require us to deprecate queryAsTransaction and executeAsTransaction since they can be replaced with the new API. What do you think of this interface?

TODO: Add retry mechanism. Just sharing the PR to get feedback on the interface.

[Liren Tu (tuliren)]

The new API makes sense.

Nitpick: seems more concise to call it withRetry, or withMaxRetry or allowRetry instead of withNumRetries.

[Sherif A. Nada (sherifnada)]

Liren Tu (@tuliren) this should be ready for more thorough review

[Liren Tu (tuliren)]

Sherif A. Nada (@sherifnada), the execution with retry looks good. However, there is a concurrency issue. Transactor should be thread safe. However, currently if multiple threads try to run different queries, they will all be manipulating the same execution context builder, and their configurations will be overridden by each other. I think one solution is to make the transactor immediately returns an execution context whenever asTransaction or withMaxRetry is called. Subsequent configurations will only affect the returned execution context.

[Sherif A. Nada (sherifnada)]

Ah, great point. I'll make sure to add tests for that. How would you feel about making ExecutionContext ThreadLocal ? It would maintain the current fluent interface without breaking thread safety.

[Liren Tu (tuliren)]

I think ThreadLocal should probably work. However, I feel that it is unnecessary to introduce that, and safer to just use separate execution context objects. One weak rationale is Occam's razor. The other one is that within one workflow, we commonly pass one transactor around into different components. It is possible that these components run in the same thread, while their configurations can interfere with each other if it is ThreadLocal.

The interface should still be fluent, because we can still write like this:

transactor.withMaxRetry(3).asTransaction().execute(...);

To make it even more fluent, ExecutionContext can extend from ITransactor. So it is just a subclass, and user will have no idea that the implementation has changed so that the following code is possible:

ITransactor<IRlDb> retryTransactor = transactor.withMaxRetry(3);
...
retryTransactor.execute(...);

[Davin Chia (davinchia)]

Closing this beautiful PR in favor of #261 .