Proposal: Move forwarders into io.grpc.util
- Lingua principale
- Java
- Stelle
- 12.1k
- Fork
- 4k
- Merge medio
- 2g 17h
- PR unite (30g)
- 37
Descrizione
We have a couple classes in the top io.grpc. package that are "Forwarding" classes. These classes are generally useful on their own, and provide an interception mechanism for changing or monitoring the behavior of the forwardee. While users could implement forwarders themselves, it would be better if we did it. There are a couple reasons:
* Forwarding is error prone. As the classes we have evolve, we continually add methods that should have been abstract but cannot be due to backwards compatibility. `ManagedChannel` is one such class. Because they are not abstract, they are easy to forget to override, as has happened in the past. We are in a position to update the forwarding class as we modify the original class. Additionally, we can take on the burden of testing that each method is forwarded more easily than users.
* Forwarders act as good alternatives to mocks. Often in tests we use mocks for ease of use. This has proven to make our tests brittle and hard to understand. It would be better to make dummy classes that are suitable for test use. Forwarders act as a good base class for tests to extend. It allows an inplace migration from mocks to not mocks. Forwarders can delegate to a mock as the test is being updated. Traditionally we haven't moved test helper classes to a shared spot, but this is due to inconvenience rather than it being bad idea.
In #3105 the issue was raised that NoopClientCall and BaseClientCall were prone to abuse. Their original intent was a stand in for mocks, as many callsites needed a nonnull instance. However, because their default behavior was "do nothing", they were more easily abused. I believe forwarding classes neatly solve both problems. A forwarder can wrap a `null` instance to be used at call sites that expect a non null instance. If any methods are called, they will fail with NPE preventing users from abusing the behavior. They can define the methods they only need for their tests instead.
The reason I raise this is that there may be a lot of Forwarders if we adopt this pattern. I think they are useful in both test code and in regular code, so the logical place to put them would be in a place accessible to both. `io.grpc.util` seems appropriate, because they are supplemental to the core library.
As for the existing Forwarding classes, they will become shim classes, and their implementation will move to io.grpc.util.
As for which new Forwarders would live in the new location, the following have been on my wish list (with the first one being recently fulfilled by @dapengzhang0 )
* ForwardingManagedChannelBuilder
* ForwardingManagedChannel (people mock this class today!)
* ForwardingServerBuilder
* ForwardingLoadBalancer
Each of these would be useful to either users and/or ourselves.
@ejona86 @zhangkun83 @zpencer thoughts?
Guida per i contributori
Apri la guida per i contributori
Direzione di ricerca
Start by inventorying the existing Forwarding classes in the top-level io.grpc package and reviewing the discussion in #3105. Compare the proposed io.grpc.util location with the listed candidates, including ForwardingManagedChannelBuilder, ForwardingManagedChannel, ForwardingServerBuilder, and ForwardingLoadBalancer. The issue is complete only when the relocation and desired additions are agreed and implemented, but it does not define a concrete scope.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Valutazione
- Stack tecnologico
- java
- Ambito
- backend-api-design
- Tipo di issue
- Funzionalità
- Difficoltà
- 5/5
- Tempo stimato
- Più di una settimana
- Stato di attività
- Ferma
- Chiarezza
- Da chiarire
- Idoneità per principianti
- 25/100