copy initial proposed API and readme from sandbox

[njr-11]

pull fixes #3
Signed-off-by: Nathan Rauh nathan.rauh@us.ibm.com

[FroMage]

LGTM: we can start with that and work through remaining issues afterwards.

[Emily-Jiang]

Should ALL be the default?

[njr-11]

The prior default was chosen to match the EE Concurrency requirement for only a specific set of context types to be propagated by default, where
@Inject @ManagedExecutorConfig ManagedExecutor executor1;
would create an instance with the same context propagation as
@Inject ManagedExecutor executor2;
which would also have the same context propagation as EE Concurrency's default instance,
@Resource ManagedExecutorService executor3;
The idea was to allow a container that is both MP Concurrency and EE Concurrency to return the same instance from all of these.

That said, it would be possible to instead default to ALL and require the use of different instance than the EE Concurrency default instance if that would be more useful.

[njr-11]

Thinking about this some more, defaulting to ThreadContext.ALL would mean that if someone implemented a provider for transaction context that was capable of propagating transactions, then those would also propagate by default. I don't think we want to go that far. It seems better for performance and predictability that we default to the more limited set of core contexts. The user can always specify ALL if they want it.

[Emily-Jiang]

Thinking about this some more, defaulting to ThreadContext.ALL would mean that if someone implemented a provider for transaction context that was capable of propagating transactions, then those would also propagate by default.

If an user provide a transaction context, surely the default one will be overriden. You won't need two of them.

[njr-11]

The 'someone' that I was referring wasn't intended to mean the end user. I was referring to a container implementation of transactions (the transaction manager). In the event that an end user did provide a conflicting implementation of one of already-provided context types, I think we discussed that would be considered an error path for now. Allowing an override of context provided by someone else seems outright dangerous, and in the case of transactions would probably lead to data integrity issues.

[Emily-Jiang]

I would say this is a controller. How about ThreadContextController?

[njr-11]

I like how the name ThreadContextController starts with ThreadContext in the same way that ThreadContextProvider and ThreadContextSnapshot do, which will cause them to naturally be listed together when interfaces are shown alphabetically. ActiveThreadContext seems more descriptive/clear, except could be confusing after it is ended, so I'm willing to change it as you suggest.

[Emily-Jiang]

There should a method e.g. getState() to return whether it is active or not. end() can check whether it is active or not. If it is, end it. If not, ignore it.

[njr-11]

Sure, we can add this, maybe as a boolean isEnded() given that there are only 2 states.

[Emily-Jiang]

Should this be String, String? Maybe String, Object has more coverage.

[njr-11]

It is Map<String, String> for compatibility with EE Concurrency execution properties. Switching to Object would force code to deal with unwanted casting.

[Emily-Jiang]

Can you put this method to ThreadContextController?

[njr-11]

Leaving it here enforces the contract that each ActiveThreadContext/ThreadContextController represents exactly one contextualized execution. If we move it, then the implementer must deal with the possibility that the user tries to invoke begin multiple times on the same instance, or tries to re-begin one that has already ended. I can't see a reason to place this additional burden/lack of clarity upon the provider of thread context without any clear benefit that would be achieved by doing so.

[Emily-Jiang]

@njr-11 please take a look at my comments.

[njr-11]

@Emily-Jiang thanks for the review. I have added a pull with several code review fixes (per comments above) as well as a couple of spelling and punctuation fixes that Nathan M and myself spotted while proofreading.

[FroMage]

Actually it may be more than one line, each with a different implementation of the interface.

[njr-11]

Thanks - good catch. We definitely want to allow for multiple. I have added a commit correcting the language here to indicate at least one instead of single.

[FroMage]

Can we please merge these two PRs ASAP and fix whatever remaining issue afterwards? It really doesn't help implementation to keep these in PRs.

[FroMage]

Thanks!

[FroMage]

Note that this prevents implementing this interface as a @FunctionalInterface as I had before:

	public ThreadContextSnapshot currentContext(Map<String, String> props) {
		MyContext capturedContext = MyContext.get();
		return () -> {
			MyContext movedContext = MyContext.get();
			MyContext.set(capturedContext);
			return () -> {
				MyContext.set(movedContext);
			};
		};
	}

[njr-11]

Thank you for spotting this as well. I was previously willing to accommodate the request to have an isEnded method when I saw no benefit either way for including or not including it. However, as you have pointed out, this makes it no longer be a functional interface. I have added a commit to remove the isEnded method.

[FroMage]

Yeah, I noticed it by accident, but I thought it made a good argument in favour of not having that extra method. I suppose we should also add @FunctionalInterface to the interfaces (at least if/until we decide to add methods to it) to make sure that we don't add any new method without just cause.

[njr-11]

Good idea. @FunctionalInterface is added to both ThreadContextController and ThreadContextSnapshot. I think as long as they are able to remain functional interfaces for the initial release, they should hopefully remain that way. We won't want to cause a lot of churn in the SPI once implementations of it start to be provided.