core: channel connectivity state API and implementation by TransportSet #2286
Conversation
| if (state != source) { | ||
| executor.execute(callback); | ||
| } else { | ||
| callbacks.add(new StateCallbackEntry(callback, executor)); |
There was a problem hiding this comment.
Since we have abstraction for this, we could reuse runInExecutor:
StateCallbackEntry callbackEntry = new StateCallbackEntry(callback, executor);
if(state != source){
callbackEntry.runInExecutor();
} else {
callbacks.add(callbackEntry):
}
| */ | ||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/28") | ||
| public void notifyWhenStateChanged( | ||
| Runnable callback, Executor executor, ConnectivityState source) { |
There was a problem hiding this comment.
nit: might place source first, since callback will commonly be an anonymous class.
| * @throws UnsupportedOperationException if not supported by implementation | ||
| */ | ||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/28") | ||
| public ConnectivityState getState(boolean requireConnection) { |
There was a problem hiding this comment.
"require" is a bit strong. How about wantConnection or requestConnection?
There was a problem hiding this comment.
+1 for requestConnection
| // Swap out callback list before calling them, because a callback may register new callbacks, | ||
| // if run in direct executor, can cause ConcurrentModificationException. | ||
| ArrayList<StateCallbackEntry> savedCallbacks = callbacks; | ||
| callbacks = new ArrayList<StateCallbackEntry>(); |
There was a problem hiding this comment.
Might be nice to lazily create the array or form a linked list out of the entries, but I guess it doesn't matter much.
There was a problem hiding this comment.
Changed to create lazily.
|
|
||
| /** | ||
| * Registers a one-off callback that will be run if the connectivity state of the channel diverges | ||
| * from the given {@code source}. If the states are already different, the callback may be called |
| * | ||
| * @param callback the one-off callback | ||
| * @param executor the executor to run the callback | ||
| * @param source typically what has just been returned by {@link #getState} |
There was a problem hiding this comment.
Maybe describe source a bit more first:
the currently assumed state, typically just returned by {@link #getState}
| * RPCs for this period, channels that are READY or CONNECTING switch to IDLE. Additionaly, | ||
| * channels that receive a GOAWAY when there are no active or pending RPCs should also switch to | ||
| * IDLE to avoid connection overload at servers that are attempting to shed connections. We will | ||
| * use a default IDLE_TIMEOUT of 300 seconds (5 minutes). |
There was a problem hiding this comment.
I'm not sure the default idle timeout should be specified here, but all the same, wasn't the talk of using like 30mins now and maybe reducing it later?
There was a problem hiding this comment.
Right. I didn't pay attention when copying the texts. Removed the default value completely.
|
@ejona86 Do you think we should require an executor on |
|
In general, I'm fine with removing the Although for LB, it probably wants to use direct and not the application's executor. But it's also probably safe to use the application's executor. |
|
All comments addressed. |
Add connectivity state methods on ManagedChannel. Make TransportSet a ManagedChannel and implement channel state API.
zhangkun83
force-pushed
the
transportset_channel_state_api_0
branch
from
e31b0da
to
107fa8e
Compare
This is the first step of #28. Spec is here.
The state API is added to ManagedChannel. TransportSet is changed to extend ManagedChannel, which would allow a LoadBalancer to 1) listen to the connectivity state of a TransportSet, and 2) shutdown a TransportSet, as required by the upcoming new LoadBalancer API outlined in #1600.
Please review the PR as a whole.