Skip to content

Bundle keep-up requests for v1.x#170

Closed
dmdashenkov wants to merge 15 commits into1.x-devfrom
bundle-keep-up-retrofit
Closed

Bundle keep-up requests for v1.x#170
dmdashenkov wants to merge 15 commits into1.x-devfrom
bundle-keep-up-retrofit

Conversation

@dmdashenkov
Copy link
Contributor

@dmdashenkov dmdashenkov commented Oct 12, 2021

In this PR we introduce a servlet and client capabilities for batch keep-up requests.

Previously, subscription keep-up requests were sent to the server per subscription, i.e. if the client had one hundred active subscriptions, the server would receive one hundred keep-up requests.

Now, we bundle all those requests into one, so that the network overhead is smaller and the number of HTTP requests to the server is lower.

The network traffic used for such a request is marginally smaller. We still send all the same data to the server and back, but in one fat exchange instead of many small exchanges.


This is a patch to the 1.x version. The "live" 2.x version will receive a similar change but with more structural refactoring as well as a network load improvement over this version.

This PR will not be merged. When the changes are reviewed, I will publish them manually, set up Git tags for the new version, and remove both branches.

@dmdashenkov dmdashenkov self-assigned this Oct 12, 2021
|| '/subscription/keep-up-all';
const request = new Subscriptions()
request.setSubscriptionList(subscriptions);
const typed = TypedMessage.of(request)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W033: Missing semicolon.
(at-me in a reply with help or ignore)

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #170 (012ae66) into 1.x-dev (234c2f4) will decrease coverage by 0.95%.
The diff coverage is 1.92%.

@@              Coverage Diff              @@
##             1.x-dev     #170      +/-   ##
=============================================
- Coverage      60.61%   59.66%   -0.96%     
  Complexity       214      214              
=============================================
  Files             93       95       +2     
  Lines           2402     2442      +40     
  Branches          45       46       +1     
=============================================
+ Hits            1456     1457       +1     
- Misses           935      974      +39     
  Partials          11       11              

@dmdashenkov dmdashenkov requested a review from armiol October 13, 2021 11:34
@dmdashenkov dmdashenkov marked this pull request as ready for review October 13, 2021 11:34
@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.
@YegorUdovchenko, FYI.

@dmdashenkov
Copy link
Contributor Author

@armiol, PTAL.

Copy link
Contributor

@armiol armiol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmdashenkov LGTM in general. Please see my comments.

* @param {!Array<spine.client.Subscription>} subscriptions subscriptions that are prevented
* from being closed by the server
* @return {Promise<Object|SpineError>} a promise of a successful server response JSON data,
* rejected if the client response is not 2xx or a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`2xx`

* {@link SubscriptionBridge}.
*
* @param bridge
* the subscription bridge to be used in to cancel subscriptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, we don't need this "in".

private final SubscriptionBridge<?, ?, ?> bridge;

/**
* Creates a new instance of {@code SubscriptionBulkKeepUpServlet} with the given
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move "the given" to the next line.


/**
* Keep up the subscription, prohibiting it from closing from the server-side.
* Keep up the subscription, preventing it from closing from the server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Keeps up".

Please address the similar issue in all docs below.

import static com.google.common.base.Preconditions.checkNotNull;

/**
* An abstract servlet for a client request to keep up an existing {@link Subscription}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"An abstract servlet handling the bulk {@link Subscription} keep-up requests."?

* {@link SubscriptionBridge}.
*
* @param bridge
* the subscription bridge to be used to keep-up subscriptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's settle down on whether we say "to keep up" or "to keep-up".

To me, "keep-up" is a noun and, maybe, an adjective. "To keep up" is a verb.

@dmdashenkov
Copy link
Contributor Author

The changes have been published under the version 1.7.4.

@dmdashenkov dmdashenkov deleted the bundle-keep-up-retrofit branch October 18, 2021 09:02
@armiol armiol changed the title Bundle keep-up requirests for v1.x Bundle keep-up requests for v1.x Jan 4, 2023
armiol added a commit that referenced this pull request Jan 5, 2023
…-1.7.4

[1.x] Bulk keep-up and cancellation requests (port of #170)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants