[FLINK-16391][http] Add DelayedMessages and EgressMessages#43
[FLINK-16391][http] Add DelayedMessages and EgressMessages#43igalshilman wants to merge 7 commits intoapache:masterfrom
Conversation
tzulitai
left a comment
tzulitai
left a comment
There was a problem hiding this comment.
Thanks @igalshilman, overall this looks good.
Some minor comments to be addresses before this is merged.
| // from the egress_identifier. | ||
| message EgressMessage { | ||
| // The target egress id in the form of <namespace>/<name> | ||
| string egress_identifier = 1; |
There was a problem hiding this comment.
Should we be consistent with the Address message, where the namespace and name are 2 separate fields?
There was a problem hiding this comment.
Either that, or we change the Address message to use the Python / YAML way of formatting namespace + name pairs: <namespace>/<name> and do the parsing in the RequestReplyFunction
| PersistedTable.of("states", String.class, byte[].class); | ||
|
|
||
| private final SingleThreadedLruCache<String, EgressIdentifier<Any>> egressIdentifierCache = | ||
| new SingleThreadedLruCache<>(16); |
There was a problem hiding this comment.
nit: move the 16 to a named class static final variable.
| functionUnderTest.invoke(context, Any.getDefaultInstance()); | ||
|
|
||
| // A message returned from the function | ||
| // that asks to put "hello" into the session state. |
There was a problem hiding this comment.
the comments here is not coherent with what is being done in the test.
| functionUnderTest.invoke(context, Any.getDefaultInstance()); | ||
|
|
||
| // A message returned from the function | ||
| // that asks to put "hello" into the session state. |
There was a problem hiding this comment.
same here: this comment is not coherent with the code
|
+1, LGTM now. |
2 participants
This PR adds the following functionality to the RequestReply protocol:
functionality: