core,netty: split stream state on client-side; AbstractStream2 #2140
Conversation
| * Requests up to the given number of messages from the call to be delivered. This should end up | ||
| * triggering {@link TransportState#requestMessagesFromDeframer(int)} on the transport thread. | ||
| */ | ||
| void request(int numMessages); |
There was a problem hiding this comment.
nit: seems a bit strange to call request on something called a Sink
There was a problem hiding this comment.
The other class is called TransportState. Perhaps a better name for this interface would be ApplicationState?
There was a problem hiding this comment.
Implementations of this interface don't generally hold interesting state. The application state is held by Abstract{Client,}Stream2.
The reason for this interface is to simplify the naming issues we were having and make it more obvious what calls what.
There was a problem hiding this comment.
So then doesn't the same comment apply to TransportState? Perhaps both should be renamed to be more consistent?
There was a problem hiding this comment.
I don't really follow. Rename Abstract{Client,}Stream2 to ApplicationState? Rename TransportState to... ???Sink? Rename both? There's three classes/interfaces involved and I don't understand what pair would gain what advantage.
I don't see the lack of consistency here, because no class is a true inverse of another. I can agree that TransportState is a bit of a weak name, but it gets the point across pretty well and I'm not aware of a better name.
At some point we could introduce Sinks to TransportState. I didn't because it seemed to add complexity without actually simplifying anything, because the various methods are pretty intertwined (especially w.r.t. error handling, since the transport has to be notified).
There was a problem hiding this comment.
No, I'm not suggesting that you rename the stream, itself. I'm suggesting that Sink/TransportState are two views of the stream, each performs ~half the work of the stream, and that they should have consistent names to make this more obvious. Names like ApplicationView and TransportView would make this clear in my mind.
There was a problem hiding this comment.
I'm suggesting that Sink/TransportState are two views of the stream
But... it's not. Sink holds no state. Sink is only to make it obvious how the abstract class interacts with the subclass, and that those method have no need to interact with the rest of the stream state.
Names like ApplicationView and TransportView would make this clear in my mind.
I can't see any way I would use "View". View would seem to imply that the state is shared. That's exactly the opposite of what I want to portray.
If I called something ApplicationState it would be AbstractClientStream2. But that seems a much poorer name than its current name (even assuming that it was tweaked as necessary to ClientStreamApplicationState).
|
LGTM. I like your tests :). |
|
Would anyone like more time to review? I expect there is still the variable name discussion that Nathan brought up. Is there anything else outstanding?
... what about them? |
don't be scared :). I think there is an elegance to |
|
I will |
|
Derp, that button is too close. |
| super(maxMessageSize); | ||
| } | ||
|
|
||
| protected abstract void http2ProcessingFailed(Status status, Metadata trailers); |
There was a problem hiding this comment.
Could you add a javadoc to abstract methods?
|
@carl-mastrangelo, okay, sounds good, at your leisure. |
| * @param frame the received data frame | ||
| * @param endOfStream {@code true} if there will be no more data received for this stream | ||
| */ | ||
| protected void transportDataReceived(ReadableBuffer frame, boolean endOfStream) { |
|
Is this PR ready for merge (once rebased), or any discussion points outstanding? |
|
It's waiting for @carl-mastrangelo to finish review. The past couple weeks have been very busy for me, so I also haven't been pushing hard. |
|
I'll get to it by eod |
|
|
||
| @Override | ||
| public Integer parseAsciiString(String serialized) { | ||
| return Integer.parseInt(serialized.split(" ", 2)[0]); |
There was a problem hiding this comment.
make sure this merges properly with upstream. I can't remember if I converted this to a trusted marshaller
|
@ejona86 LGTM. Please take care that the rebase goes successfully, there are a number of places that are likely going to conflict. |
ejona86
force-pushed
the
split-abstractclientstream
branch
from
e38a687
to
f3320b6
Compare
|
@carl-mastrangelo, PTAL. The merge conflicts you can't really tell, since they are all together with everything else. But they were more mundate like imports or changes that happened on adjacent lines. The changes you pointed out didn't conflict at all, since the similarity of the copied files isn't high enough for git to notice the copy. So I had to just re-apply them manually. I would have squashed in the JavaDoc commit, but it appears I never pushed it out for others to see, so here it is now. |
|
@ejona86 Still LGTM |
OkHttp will need to be migrated in a future commit.
ejona86
force-pushed
the
split-abstractclientstream
branch
from
f3320b6
to
cad7124
Compare
OkHttp will need to be migrated in a future commit.
This is a continuation of what already was done for server-side (#1656). Although
AbstractClientStream2 looks like new code, it is a reorganization of existing code
(although heavily reworked).