Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING CHANGE: refactor(subscriber): remove unneeded code & utilize typescript #388

Merged
merged 24 commits into from
Jan 14, 2019
Merged

BREAKING CHANGE: refactor(subscriber): remove unneeded code & utilize typescript #388

merged 24 commits into from
Jan 14, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Dec 5, 2018

Fixes #6
Fixes #12
Fixes #115
Fixes #119

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Breaking change ahead!

There's been a small restructuring made to the subscription options.

Before:

const subscription = topic.subscription('my-sub', {
  batching: {
    maxMilliseconds: 100,
  },
  flowControl: {
    maxBytes: os.freem() * 0.2,
    maxMessages: 100,
  },
  maxConnections: 5,
});

After

const subscription = topic.subscription('my-sub', {
  ackDeadline: 10,
  batching: {
    callOptions: {}, // gax call options
    maxMessages: 3000,
    maxMilliseconds: 100,
  },
  flowControl: {
    allowExcessMessages: true,
    maxBytes: os.freem() * 0.2,
    maxExtension: Infinity,
    maxMessages: 100
  },
  streamingOptions: {
    highWaterMark: 0,
    maxStreams: 5, // formerly known as maxConnections
    timeout: 60000 * 5, // 5 minutes
  }
});

@callmehiphop callmehiphop added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 5, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 5, 2018
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but I don't have a great feel for the logic of the stuff you're replacing :)

src/lease-manager.ts Outdated Show resolved Hide resolved
src/message-queues.ts Outdated Show resolved Hide resolved
src/lease-manager.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@stephenplusplus stephenplusplus left a comment

Choose a reason for hiding this comment

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

Nice work! I really like the new hierarchy 👍

src/lease-manager.ts Outdated Show resolved Hide resolved
src/lease-manager.ts Outdated Show resolved Hide resolved
src/lease-manager.ts Outdated Show resolved Hide resolved
src/message-queues.ts Outdated Show resolved Hide resolved
src/lease-manager.ts Outdated Show resolved Hide resolved
}

this._acks.add(message);
await this._acks.onFlush();

This comment was marked as spam.

This comment was marked as spam.

src/subscriber.ts Outdated Show resolved Hide resolved
src/message-stream.ts Outdated Show resolved Hide resolved
src/message-stream.ts Outdated Show resolved Hide resolved
src/message-stream.ts Outdated Show resolved Hide resolved
@JustinBeckwith
Copy link
Contributor

👋 @callmehiphop I'm super excited for this to land :)

@callmehiphop callmehiphop removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 8, 2019
@callmehiphop
Copy link
Contributor Author

@JustinBeckwith @stephenplusplus finished tests and did a few refactors here and there, but I think we're good to go!

@JustinBeckwith
Copy link
Contributor

LGTM :) If @stephenplusplus is cool, I'm cool.

src/subscription.ts Outdated Show resolved Hide resolved
this._onEnd(stream, status);
} else {
stream.once('end', () => this._onEnd(stream, status));
stream.push(null);

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

* @param {stream} stream Duplexify stream.
*/
private _interceptGrpcStream(stream: GaxDuplex): void {
const setReadable = stream.setReadable;

This comment was marked as spam.

This comment was marked as spam.

@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 10, 2019
@callmehiphop
Copy link
Contributor Author

I went ahead and made a couple perf updates. I think the one worth mentioning is the decision to bypass gax streams and use the grpc stream directly. With StreamingPull we need to buffer as few PullResponse objects as possible otherwise we run the risk of higher re-delivery rates. We will actually double the number of potential objects buffered by using gax since it wraps the grpc stream in a Duplex stream, adding another readable buffer to our pipeline. Currently gax does not offer any enhancements (retries, etc.) to BIDI streams, so this change should be a win/win scenario.

@JustinBeckwith JustinBeckwith changed the title refactor(subscriber): remove unneeded code & utilize typescript BREAKING CHANGE: refactor(subscriber): remove unneeded code & utilize typescript Jan 14, 2019
@JustinBeckwith JustinBeckwith added the semver: major Hint for users that this is an API breaking change. label Jan 14, 2019
@JustinBeckwith JustinBeckwith merged commit b6766ef into googleapis:master Jan 14, 2019
This was referenced Apr 29, 2020
@feywind feywind mentioned this pull request May 20, 2020
@release-please release-please bot mentioned this pull request May 20, 2020
feywind pushed a commit to feywind/nodejs-pubsub that referenced this pull request Nov 12, 2024
* build: add Kokoro configs for autorelease

* build: add Kokoro configs for autorelease

* chore: remove CircleCI config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. semver: major Hint for users that this is an API breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants