The pick_first policies in core and grpclb previously would call
Subchannel.requestConnection() from data-path. They now will schedule
that call in the sync-context to avoid the warning. They will only
call it for the first pick of each picker, to prevent storming the
sync-context.
This is a revised version of #5503 (62b03fd), which was rolled back in f8d0868. The newer version passes SubchannelStateListener to Subchannel.start() instead of SubchannelCreationArgs, which allows us to remove the Subchannel argument from the listener, which works as a solution for #5676.
LoadBalancers that call the old createSubchannel() will get start() implicitly called with a listener that passes updates to the deprecated LoadBalancer.handleSubchannelState(). Those who call the new createSubchannel() will have to call start() explicitly.
GRPCLB code is still using the old API, because it's a pain to migrate the SubchannelPool to the new API. Since CachedSubchannelHelper is on the way, it's easier to switch to it when it's ready. Keeping
GRPCLB with the old API would also confirm the backward compatibility.
We will require Subchannel.requestConnection() to be called from
sync-context (#5722), but SubchannelPicker.requestConnection() is
currently calling it with the assumption of thread-safety. Actually
SubchannelPicker.requestConnection() is called already from
sync-context by ChannelImpl, it makes more sense to move this method
to LoadBalancer where all other methods are sync-context'ed, rather than
making SubchannelPicker.requestConnection() sync-context'ed and fragmenting
the SubchannelPicker API because pickSubchannel() is thread-safe.
C++ also has the requestConnection() equivalent on their LoadBalancer
interface.
As we are now endorsing the wrapping of ClientStreamTracers by
providing ForwardingClientStreamTracer, there is a need for altering
StreamInfo, especially CallOptions before it's passed onto the
delegate. A Builder class and a toBuilder() provides a robust way
to copy the rest of the fields.
This is a breaking change for anybody who creates StreamInfo, which is
unlikely in non-test code, because StreamInfo was added as late as
1.20.0.
io.grpc has fewer dependencies than io.grpc.internal. Moving it to a
separate artifact lets users use the API without bringing in the deps.
If the library has an optional dependency on grpc, that can be quite
convenient.
We now version-pin both grpc-api and grpc-core, since both contain
internal APIs.
I had to change a few tests in grpc-api to avoid FakeClock. Moving
FakeClock to grpc-api was difficult because it uses
io.grpc.internal.TimeProvider, which can't be moved since it is a
production class. Having grpc-api's tests depend on grpc-core's test
classes would be weird and cause a circular dependincy. Having
grpc-api's tests depend on grpc-core is likely possible, but weird and
fairly unnecessary at this point. So instead I rewrote the tests to
avoid FakeClock.
Fixes#1447
Resolves#5497
## Motivation
In hierarchical `LoadBalancer`s (e.g., `XdsLoadBalancer`) or wrapped `LoadBalancer`s (e.g., `HealthCheckingLoadBalancerFactory`, the top-level `LoadBalancer` receives `Subchannel` state updates from the Channel impl, and they almost always pass it down to its children `LoadBalancer`s.
Sometimes the children `LoadBalancer`s are not directly created by the parent, thus requires whatever API in the middle to also pass Subchannel state updates, complicating that API. For example, the proposed [`RequestDirector`](https://github.com/grpc/grpc-java/issues/5496#issuecomment-476008051) includes `handleSubchannelState()` solely to plumb state updates to where they are used. We also see this pattern in `HealthCheckingLoadBalancerFactory`, `GrpclbState` and `SubchannelPool`.
Another minor issue is, the parent `LoadBalancer` would need to intercept the `Helper` passed to its children to map Subchannels to the children `LoadBalancer`s, so that it pass updates about relevant Subchannels to the children. Otherwise, a child `LoadBalancer` may be surprised by seeing Subchannel not created by it, and it's not efficient to broadcast Subchannel updates to all children.
## API Proposal
We will pass a `SubchannelStateListener` when creating a `Subchannel` to accept state updates, those updates could be directly passed to where the `Subchannel` is created, skipping the explicit chaining in the middle.
Also define a first-class object `CreateSubchannelArgs` to pass arguments for the reasons below:
1. It may avoid breakages when we add new arguments to `createSubchannel()`. For example, a `LoadBalancer` may wrap `Helper` and intercept `createSubchannel()` in a hierarchical case. It may not be interested in all arguments. Passing a single `CreateSubchannelArgs` will not break the parent `LoadBalancer` if we add new fields later.
2. This also reduces the eventual size of Helper interface, as the convenience `createSubchannel()` that accepts one EAG instead of a List is no longer necessary, since that convenience is moved into `CreateSubchannelArgs`.
```java
interface SubchannelStateListener {
void onSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState);
}
abstract class LoadBalancer.Helper {
abstract Subchannel createSubchannel(CreateSubchannelArgs args);
}
final class CreateSubchannelArgs {
final List<EquivalentAddressGroup> getAddresses();
final Attributes getAttributes();
final SubchannelStateListener getStateListener();
final class Builder () {
...
}
}
```
The new `createSubchannel()` must be called from synchronization context, as a step towards #5015.
## How the new API helps
Most hierarchical `LoadBalancer`s would just let the listener from the child `LoadBalancer`s directly pass through to gRPC core, which is less boilerplate than before.
Without any effort by the parent, each child will only see updates for the Subchannels it has created, which is clearer and more efficient.
If a parent `LoadBalancer` does want to look at or alter the Subchannel state updates for its delegate (like in `HealthCheckingLoadBalancerFactory`), it can still do so in the wrapping `LoadBalancer.Helper` passed to the delegate by intercepting the `SubchannelStateListener`.
## Migration implications
Existing `LoadBalancer` implementations will continue to work, while they will see deprecation warnings when compiled:
- The old `LoadBalancer.Helper#createSubchannel` variants are now deprecated, but will work until they are deleted. They create a `SubchannelStateListener` that delegates to `LoadBalancer#handleSubchannelState`.
- `LoadBalancer#handleSubchannelState` is now deprecated, and will throw if called and the implementation doesn't override it. It will be deleted in a future release.
The migration for most `LoadBalancer` implementation would be moving the logic from `LoadBalancer#handleSubchannelState` into a `SubchannelStateListener`.
This class is used in other places than just NameResolver.Helper. It
should not be an inner class of Helper.
Strictly speaking this is an API-breaking change. However, this is
part of the service config error handling API that hasn't been done
yet. Nobody has a legitimate reason to use it.
This mainly avoids protoc from 3.7.0 which has a dependency on libatomic. Most
of our systems have libatomic, so it mostly works, but the interop docker
container does not, so building fails. Version 3.7.1 was rebuilt to avoid
needing the libatomic shared library.
This has the added benefit that Bazel is now on the same version as Gradle, as
3.7.1 included fixes for Bazel.
* grpclb: use a stopwatch to manage elapased time for LB backoff
* reformatt lines
* create a new Stopwatch for each single newly created HC stream
* use existing stopwatch supplier provided by FakeClock in tests
The problem: GrpclbState tracks Subchannels' states as a mutable
attribute in Subchannel.getAttributes(). However, GrpclbState only
update this attribute for the Subchannels its managing. For those
cached in SubchannelPool, their state attributes are stale. When they
are given back to GrpclbState, IDLE state is assumed. As a result, if
a Subchannel is READY when it's reclaimed from the pool, it will not
be picked.
To fix that, this change expands SubchannelPool interface to handle
Subchannel state updates, which GrpclbState will call. SubchannelPool
saves the latest state and delivers it when it's returned to
GrpclbState by scheduling a call to handleSubchannelState() in the
SynchronizationContext, so that GrpclbState will take the latest state
as if it was just reported from the Channel.
The PICK_FIRST mode puts all backend addresses in a single Subchannel. There are a few points where it's different from the default ROUND_ROBIN mode:
1. PICK_FIRST doesn't eagerly connect to backends like ROUND_ROBIN does. Instead, it requests for connections when the Subchannel is picked.
2. PICK_FIRST adds tokens to the headers via a different code path (`TokenAttachingTracerFactory`) than ROUND_ROBIN
3. For simple implementation, when the mode is changed by service config when the LoadBalancer is working, we will shut down `GrpclbState` and starts a new one with the new mode. All connections will be closed during the transition. We don't expect this to happen in practice given the specific use case of PICK_FIRST.
Make sure the config for grpclb is passed to the GrpclbLoadBalancer, which will support two child policies -- "round_robin" (default) and "pick_first".
Previously the presence of balancer addresses would dictate "grpclb" policy, despite of the service config. Service config will now take precedence instead.
Implement config parsing logic in GrpclbLoadBalancer. Per offline discussions with @markdroth and @ejona86, we will ignore configuration errors for now. The more appropriate config error handling is upcoming.
This commit swaps to using a Sync task to place generated code in the
src/generated folder instead of the gradle-protobuf-plugin's
generatedFilesBaseDir. This provides much nicer results on failed
builds, and you will no longer see all the generated files deleted.
But at the same time the Sync task makes it easy to only copy the
grpc-generated code. This was not previously done because we were lazy
and using generatedFilesBaseDir, which made it difficult to treat the
services differently from the messages.
This will be a new override. The old override is now deprecated.
In order to pass new information without adding new overrides, I shoved most information
into an object called StreamInfo. The Metadata is left out to draw attention because
it's mutable.
Motivation: this is needed for correctly supporting pick_first in GRPCLB. GRPCLB needs to
add a token to the headers, and the token varies among servers. With round_robin, GRPCLB
create a Subchannel for each server, thus can attach the token when the Subchannel is picked.
To implement pick_first, all server addresses will be put in a single Subchannel, we will
need to add the header in newClientStreamTracer(), by looking up the server address from
the transport attributes and deciding which token to add.
For Bazel, we upgrade to protobuf 3.6.1.2 and javalite HEAD to fix
incompatibilities in newer Bazel releases.
compiler/Dockerfile is unused, so it was removed instead of being updated.
protoc no longer includes codegen for nano, so we remain on the older protoc
any time nano is used.
Protobuf now requires C++11 when compiling, so windows was swapped to
VC 14.
Remove unused variables and prefer ArrayDeque to LinkedList. The swap to
Queue from Deque was just to make it more obvious what the usage was,
since the original swap to Deque was to avoid the same LinkedList lint
violation (3d51756d).
Introduce ChannelLogger, which is a utility provided to LoadBalancer implementations (potentially NameResolvers too) for recording events to channel trace. This is immediately required by client-side health checking (#4932, https://github.com/grpc/proposal/blob/master/A17-client-side-health-checking.md) to record an error about disabling health checking. It is also useful for any LoadBalancer implementations to record important information.
ChannelLogger implementation is backed by the internal ChannelTracer/Channelz. Because Channelz limits the number of retained events, and events are lost once the process ends, I have expanded it to also log Java logger. This would provide a "last resort" in cases where there are too many events or off-line investigation is needed. All logs are prefixed with logId so that they can be easily associated with the involved Channel/Subchannel.
To prevent log spamming, the logs are all at FINE level or below so that they are not visible by default. They are logged to ChannelLogger's logger, so that user can have precise control.
There are also more verbose information that may not fit in ChannelTracer, but can be useful for debugging. It's desirable that these logs are associated with logId, but they currently manually include the logId, which is cumbersome and may result in inconsistency. For this use case, I added the DEBUG level for ChannelLogger, which formats the log in the same way as other levels, while not recorded to Channelz.
I have converted most logging and channel tracer recording in the Channel implementation and LoadBalancers.
LoadBalancerProvider is the interface that extends LoadBalancer.Factory. LoadBalancerRegistry is the one that loads the providers through service loader, and allows users to access providers through their names.
pick_first and round_robin balancer factories, which are experimental public API are now deprecated. Their providers are internal, as they are accessible by policy name.
AutoConfiguredLoadBalancerFactory is modified to access implementations purely by their names, thus hard-coded class names are no longer needed, and it can support arbitrary policy selected by service config.
Previously the client waits ~10 seconds until the fallback timer has
expired. While the timer is useful to address the long tail, it
shouldn't delay using the fallback in case of obvious errors, like the
channel failing to connect or an UNIMPLEMENTED response.
Provides a `SynchronizationContext` for scheduling tasks, with and without delay, from LoadBalancer implementations. This absorbs and extends the internal utility `ChannelExecutor`. It supersedes `Helper.runSerialized()`, which is now deprecated.
# Motivation
I see multiple cases that schedule tasks with a delay while requiring the task to run in the "Channel Executor". There have been repeated work to wrap scheduled tasks and handle races between cancellation and task run (see the diff in `GrpclbState.java` for example). The LoadBalancer implementation (e.g., GrpclbLoadBalancer) also has to acquire the `ScheduledExecutorService` from somewhere and release it upon shutdown.
The upcoming HealthCheckLoadBalancer (#4932), which would use back-off policy to retry health-checking streams, would have to do all the things above. At this point I think we need to provide something that combines `runSerialized()` with a scheduled executor with the same synchronization guarantees.
# Design details
`SynchronizationContext` is a similar to `ScheduledExecutorService` but tailored for use in `LoadBalancer` and potentially other cases outside of `LoadBalancer`. It offers task queuing and serialization and delayed scheduling. It guarantees non-reentrancy and happens-before among tasks. It owns no thread, but run tasks on caller's or caller-provided threads.
All channel-level state mutations and callback methods on `LoadBalancer` are done in a SynchronizationContext, which was previously referred to as "Channel Executor".
`SynchronizationContext.schedule()` returns a `ScheduledHandle` for status checking and cancellation. `ScheduedFuture` from `SchedulingExecutorService.schedule()` is too broad for our use cases (e.g., the blocking `get()` should never be used).
`SynchronizationContext.schedule()` requires a `ScheduledExecutorService`, which is now available through `Helper.getScheduledExecutorService()`. LoadBalancers don't need to worry about where to get `SchedulingExecutorService` any more.
# Alternatives
Alternatively, we could keep `Helper.runSerialized()` and add something like `Helper.runSerialiezdWithDelay()`, but having them on their own interface allows clean fake implementation by `FakeClock` for test, and allows other components (potentially `InternalSubchannel` for reconnection backoff) to use it too.
Instead of asking caller of `schedule()` to provide the `ScheduledExecutorService`, we considered having SynchronizationContext take a `ScheduledExecutorService` at construction. It would be inconvenient for LoadBalancer implementations that don't use `schedule()`, as they would be forced to provide a fake `ScheduledExecutorService` (which is cumbersome).
Instead of making `SynchronizationContext` a (semi-)concrete class, we considered making it an pure abstract class. However, we found it nontrivial to implement `execute()` correctly with the non-reentrancy guarantee.
This will allow enabling Error Prone on JDK 10+ (after
updating the net.ltgt.errorprone plugin), and is also a
prerequisite to that plugin update.
Also remove net.ltgt.apt plugin, as Gradle has native
support for annotationProcessor.
Those overrides are kept for backward compatibility and convenience
for callers. Documentation already says implementations should not
override them. Making them final reduces confusion around which
override should be verified in tests and be overridden in forwarding
classes, thus prevents bugs caused by such confusion.
The addresses from the string dump of the LoadBalanceResponse proto is
in binary format and not human-readable. We will log the
BackendAddressGroups when using a new list from the balancer. The
original logging of LoadBalanceResponse is downgraded to FINER level.
This is an API used to coordinate across packages and must live in
`io.grpc`.
Prepending `Internal` makes it easier to detect and hide this class
from public visibility when using certain build tools.
fixes#4796
This PR adds an automatic gradle format checker and reformats all the *.gradle files. After this, new changes to *.gradle files will fail to build if not in good format, just like checkStyle failure.
Deprecate static builder method, Keys.of(), add a notice of plans to
remove keys(), emphasize that the name is only a debug label.
The `@ExperimentalAPI` is left on the class because there are still
issues around hashCode/equals.
This is to conform with the GRPCLB spec. The spec doesn't (yet) define
the actual timeout. We choose 10 seconds here arbitrarily. It may be
configurable in the future.
This will fix internal bug b/74410243
Spies are really magical and easily produce unexpected results. Using them in
tests can easily yield tests that don't do what you think they do. Delegation
is much safer when possible.
Delegation doesn't work when methods `return true`, final methods, and with
restricted visibility, though. So CensusModulesTest and
MaxConnectionIdleManagerTest are left as-is.
The channelz service must not live in io.grpc.internal, and channelz
needs to be able to get the identifier of the entities it
tracks. Since io.grpc can not refer to io.grpc.internal, the LogId
must be moved out of internal.
Previously fallback mode can be entered only if the client has not
received any server list and the fallback timeout has expired.
Now the fallback timer is started when the stream to the balancer is broken
AND there is no ready Subchannels. Fallback mode is activated when the
timer expires. When a new server list is received from the balancer, either
the fallback timer is cancelled, or fallback mode is exited.
Also fixed a bug that the fallback timer should've been cancelled when GrpcState
is shut down.
This moves away from the global String-based Span name registry which
is not as flexible as we desire.
Also renamed the option name to be more accurate. This is not
API-breaking because the origianl addition to MethodDescriptor and
code-gen didn't make it into the 1.7.0 release.
* MethodDescriptor is lazy loaded, so protobuf loading only happens on demand. This also means tracing registration happens on demand.
* The names of the getters all being with `method`. This makes it harder for autocomplete to pick them up.
* A new field is used, which matches the getter name. Rather than make the new-getters reference the old-fields, make the old-fields reference the new getters. This makes removal of the old-fields a simple operation.
* The getters may not be inlineable, but thats an easy fix if it ends up being a problem. Not worth premature optimization (but is worth future work).
The expected timeline for this is adding this to the 1.8 cut, and deprecating the old-fields. They will be removed in 1.9.
This is a more favorable approach than #3467. Doing the registration
in MethodDescriptor should allow us to deregister in case the
generated stub and its MethodDescriptors are garbage-collected
routinely, e.g., if they are loaded by a separate ClassLoader.
Two methods, outboundMessageSent() and inboundMessageRead() are added to StreamTracer in order to associate individual messages with sizes. Both types of sizes are optional, as allowed by Census tracing.
Both methods accept a sequence number as the type ID as required by Census. The original outboundMesage() and inboundMessage() are also replaced by overrides that take the sequence number, to better match the new methods. The deprecation of the old overrides are tracked by #3460
GrpclbLoadBalancer can work in non-GRPCLB (delegate) mode according to
name resolution results. Previously the policy selection, delegation
and GRPCLB logic are in the same file, which is not very readable. It
will get worse as we going to implement policy fallback logic soon.
This PR refactors the GRPCLB logic out, and makes GrpclbLoadBalancer
focus on the policy selection and delegation logic.
This bump changelist is applied a bit late with respect to the
1.6.0 branch cut. Look at the 1.6.0 to see the source of truth of
where it was cut. Do not assume it is the commit that precedes
this one.
Previously, the round-robin list that the client uses (effective
round-robin list, ERRL) was the received round-robin list (RRRL)
excluding non-READY backends. Drop and backend entries are in the
same list.
The problem with it is that when not all backends are READY, drop
entries take a larger proportion in ERRL than they do in the RRRL,
resulting a larger drop ratio than intended.
To fix this, we employ a two-list scheme:
- A "drop list" (DL) that is out of the RRRL, with the same size and
the same number of drop entries.
- A "backend list" (BL) that contains only the backend entries from
the RRRL, excluding non-READY ones.
For every pick, the client would round-robin on the DL to determine
whether the pick should be dropped. Only when it's not dropped,
round-robin on the BL to pick the actual backend.
This way, the drop ratio is always equal to the proportion they take
in the RRRL.
Previously only when all addresses returned by NameResolver are
balancers would GRPCLB be forced. This change is a follow-up of
https://github.com/grpc/grpc/pull/10258
The equivalent of the has* methods for oneofs is to check which Case()
is used. Reworked the code a bit because it seemed obvious that it
wasn't checking all the cases it should, and is probably more natural
now that the code uses Case().
This allows avoiding re-creating connections unnecessarily. For
NameResolvers that support notifications this avoids stampeding herds
when possible, since all clients may receive the notification at
approximately the same time.
Each time helper.updatePicker() is called, the Channel will re-process
all pending streams with the new picker. If the old picker is
equivalent to the old one, it's wasteful.
This is also needed to make our internal integration test easier.
Because the load-balancer may send address list that is identical to the
previous one, just to update the TTL. Without this change, new picker
replaces the old picker even if they carry the same list, which
effectively resets the round-robin pointer. This causes a little
imbalance between test backends, resulting in test failure.
Resolves#2716
- Add attributes to EquivalentAddressGroup
- Deprecate ResolvedServerInfoGroup by EquivalentAddressGroup
- Deprecate ResolvedServerInfo, because attributes for a single address
with an address group is not found to be useful.
- The changes on the NameResolver and LoadBalancer interfaces are backward-compatible
in the next release, with which implementors can switch to the new API smoothly.
As a related change, redefine the semantics of DnsNameResolver and
RoundRobinLoadBalancer:
- Before: DnsNameResolver returns all addresses in one address group.
RoundRobinLoadBalancer ignores the grouping of addresses and
round-robin on every single addresses. It doesn't work well with the
one-server-multiple-address setup, e.g., both IPv4 and IPv6 addresses
are returned for a single serve, even if they are put in the same
address group by the NameResolver.
- After: DnsNameResolver returns every address in its own
EAG. RoundRobinLoadBalancer takes an EAG as a whole, and only
round-robin on the list of EAGs. The new behavior is a better
interpretation of the EAGs, and really allows the case where one
server has more than one addresses (e.g., IPv4 and IPv6).
This change will affect users that use custom LoadBalancer with the
stock DnsNameResolver, and those who use custom NameResolver with the
stock RoundRobinLoadBalancer.
Users who use both the stock DnsNameResolver and RoundRobinLoadBalancer
or PickFirstBalancer will see no behavioral change. Because they will
still round-robin on individual addresses from DNS, or do pick-first on
all addresses from DNS (PickFirstBalancer flattens all addresses).
The result is a simpler API and reduction of boilderplates.
The balancer service attaches a token string for each Server entry it
sends to the client. The client has to set the token to the "lb-token"
header when assigning that Server entry to an RPC.
For convenience of testing, also implemented hashCode() and equals() for
PickResult.
Besides API changes, this implementation is also up-to-date with the
latest design:
1. Delegate to round-robin and pick-first policies if requested by
the naming system.
2. OOB channels to LoadBalancer always use the LB authority provided by
the naming system.
3. Never send application RPCs to balancer addresses, even if the
address returns UNIMPLEMENTED error.
not necessary to synchronze every time calling
getServiceDescriptor(), if the descriptor has been created already;
go with the double-checked locking idom
core: adds @Nullable Object getAttachedObject() to ServiceDescriptor
compiler: Plumbing necessary to access proto file descriptors via
the reflection service
Instead of `List<List<ResolvedServerInfo>>`, `onUpdate` now takes
`List<ResolvedServerInfoGroup>` as an argument and every `ResolvedServerInfoGroup`
object can have `Attributes` attached to it which means that we can provide
attributes on each level:
* root level via `onUpdate` argument (applies to all servers)
* group level via property of `ResolvedServerInfoGroup` (applies to all servers
in the group)
* host level via property of `ResolvedServerInfo` (applies to a single server)
partially resolving #1469
The added option for java_plugin `enable_deprecated` is `true` by default in `java_plugin.cpp`, so the generated code for `TestService.java` (`compiler/build.gradle` not setting this option) has all deprecated interfaces and static bindService method.
`./build.gradle` and `examples/build.gradle` set this option explicitly to `false`, so all the other generated classes do not have deprecated code.
Will set `enable_deprecated` to `false` by default in future PR when we are ready.
first step to address issue #1469:
- leave and deprecate interfaces in codegen
- introduce `ServiceImplBase`,
- `AbstractService` is deprecated and extends `ServiceImplBase`
- static `bindService()` is deprecated
This allows us to play with zero-copy and proto3 support for lite.
Unfortunately, it introduced some warnings, so deprecated warnings are
now ignored for benchmarks and interop-testing.
This reverts commit 3df1446deb.
The commit was adding to the difficulty of integration for testing. By
itself it isn't bad, so this is a temporary revert until the many other
commits are absorbed and then it will be reapplied.
This does have a manual edit for ClientCallsTest.
Kun Zhang