Bundle keep-up #171
Bundle keep-up #171
Conversation
…subscription metadata
…te some integration tests
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
| } | ||
|
|
||
| /** Executes a command in the project [rootFolder]. */ | ||
| private fun execute(vararg command: String): String = Cli(rootFolder).execute(*command) |
There was a problem hiding this comment.
SpreadOperator: In most cases using a spread operator causes a full copy of the array to be created before calling a method which has a very high performance penalty.
(at-me in a reply with help or ignore)
|
@armiol, PTAL. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmdashenkov please see my comments, mostly on naming and documenting. Other than that, LGTM.
| this._keepUpOrGiveUp(); | ||
| } | ||
|
|
||
| _cancelClosed() { |
There was a problem hiding this comment.
I would document this one and the one below.
The first one is pretty mysterious by name. And the second requires some clarifications on that "Or".
| private final StoredJson originalJson; | ||
|
|
||
| private NodeValue(JsonObject value) { | ||
| private NodeValue(JsonObject value, @Nullable StoredJson originalJson) { |
There was a problem hiding this comment.
Please document the ctor. It's now a bit odd to see the value and the original JSON out of the context of any operation.
| return originalJson.as(cls); | ||
| } else { | ||
| String jsonMessage = value.toString(); | ||
| return fromJson(jsonMessage, cls); |
There was a problem hiding this comment.
I would put this fromJson into @see section of the method's Javadoc — as more clarification is required on how the value is parsed etc.
It will be also helpful to explain how this originalJson (which I have previously requested to document along with the ctor) plays here.
| */ | ||
| @Internal | ||
| public <M extends Message> M as(Class<M> cls) { | ||
| return Json.fromJson(value(), cls); |
There was a problem hiding this comment.
Similarly to the one above, let's reference fromJson in @see.
Also, let's have them both either statically imported, or both used with the class name. Now, it's half-and-half.
| default boolean isExpired() { | ||
| Timestamp validThru = getValidThru(); | ||
| Timestamp now = Time.currentTime(); | ||
| return compare(now, validThru) > 0; |
There was a problem hiding this comment.
Someone should explain this > (and not >=).
Ideally, that could be somewhere in the Protobuf definitions. But I cannot see any changes on that.
There was a problem hiding this comment.
I've added notes about when does the time elapse in Java and in Protobuf.
It is also hinted in the name "valid thru", (not "valid until").
It might be important for the users to understand this detail for the full picture, but, in practice, I don't think it matters much if the subscriptions become stale at the moment or after the moment. Unlike, say, delivery, this kind of granularity should not be expected, as the staleness is validated at the server but the prolongation requests are created at the client. I doubt there will be a case when this really matters to the API user.
| import "spine/client/subscription.proto"; | ||
| import "spine/core/ack.proto"; | ||
|
|
||
| // A subscription initiated by a web client. |
There was a problem hiding this comment.
Let's have a file-level comment explaining how these definitions are different from the one provided by the subscription service.
|
|
||
| // A subscription initiated by a web client. | ||
| // | ||
| // The subscription may have `extra` properties required by the web client. |
There was a problem hiding this comment.
How these properties are used? Do they characterise the subscription in case the web client "forgets"?
Please have some usage example for those. It's now a bit blurry and generic.
| // The server may add this time or another duration. The updated subscription expiration date | ||
| // is provided in the response. | ||
| // | ||
| // If the client wishes to *reduce* the lifetime of the subscription, it can do so by providing |
There was a problem hiding this comment.
Oh my god :) What if we submit something really-really negative? I.e. Duration.of(-65535, YEAR)?
I would not support negative values ever, as it will bring much more issues down the road. And — I suspect — the increased volume of the code to handle the edge cases. It is going to out-weight the benefits
— from all cases, it's probably <1% that will use these negative values.
Also, negative Durations looks really weird.
| } | ||
|
|
||
| // The response to the `Cancel` request. | ||
| message Cancelling { |
There was a problem hiding this comment.
I would name this Cancellations — it's much more clear.
| } | ||
|
|
||
| // The response to the `KeepUp` request. | ||
| message KeepingUp { |
There was a problem hiding this comment.
What if we name all responses and outcomes as events? I mean, the requests are already very commanding.
E.g. —
SubscriptionsKeptUp,
SingleSubscriptionKeptUp,
SubscriptionsCancelled.
That would eliminate all the dance with "KeepingUp" and "Cancelling" — which are really the same Outcomes, but "..damn it, the word is used already". And honestly, to me "Cancelling" is a process, not a result, or a bunch of results.
There was a problem hiding this comment.
I've applied this suggestion partially.
The "top-level" types, i.e. Subscribing, KeepingUp, and Cancelling are renamed to sound more event-ish. However, I don't want to use SingleSubscriptionKeptUp because it's a bit misleading. It might be kept up, or it not be due to an error.
In a normal command-event schema, we would throw a rejection here. I guess, the HTTP analog of that is an error code. However, the point here is to compose operations on multiple subscriptions under a single request/response pair. Thus, we return an "OK" response with an error as one of the outcomes.
|
@armiol, PTAL again. |
armiol
left a comment
armiol
left a comment
There was a problem hiding this comment.
@dmdashenkov LGTM with a single comment.
I suggest returning to the discussion on the naming when we are back to the office. It seems to be the case for brain-storming.
| * | ||
| * The server responds with status for each subscription. If a subscription could not be found on | ||
| * the server (e.g. because it was already closed), the server responds with an error. | ||
| * In this case, the client removes the subscription. The callbacks will no longer receive |
There was a problem hiding this comment.
Can we please not leave "updates" alone? Maybe, "receive" could join?
In this PR, we change the client-server interaction protocol around entity state and event subscriptions.
When a client subscribes to server updates, the server allocates some resources in order to keep the subscription working. Unfortunately, we cannot trust the client to close all the subscriptions (because of network errors, page closures, programming mistakes). Thus, the server will eventually close the subscription by itself.
In order to ensure that the subscription is alive as long as the client needs it, the client sends keep-up requests as a way of saying that the client is still there and the subscription is still relevant.
Batch requests
Previously, such requests were sent individually for each subscription. This created an unnecessary load on the server.
From now on, the requests are merged into one, so that the server can handle them in bulk. The same change is applied to the subscription cancellation requests.
Explicit lifespan
Previously, the clients figured out when to send the keep-up requests based on a convention. Now, the subscription and keep-up requests include the time for which the client needs the subscription, and the responses to those requests contain the actual expiration date & time for each subscription. So, the clients may choose to request a longer or a shorter prolongation and the server may choose to fulfill the request or set its own expiration time to a subscription no matter what.
In the current version, the JS client still sends the keep-up requests every
nseconds, effectively ignoring the available expiration date & time data. However, now there is a possibility to alter the client lib to work with that data without extra changes to the server lib.Compatibility
These changes to the protocol are breaking and aren't built to be compatible with the previous protocol. This means that the updates to the client and the server must be done in sync.
There are also breaking changes to the Java server library. In particular, the generic parameters of the subscription servlets and
SubscriptionBridgeare removed.The API of the JS client library, on the other hand, is unchanged.
Also, the 3 subscription HTTP endpoints,
Subscribe-,SubscriptionKeepUp-, andSubscriptionCancelServletare not changed. In most cases, the users will not have to alter any code around those servlets. TheSubscriptionBulkKeepUpServletand theSubscriptionCancelAllServlet, introduced in the latest patch to the v1.x, are removed.