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

Allow customized options #6

Merged
merged 1 commit into from
Nov 18, 2019
Merged

Allow customized options #6

merged 1 commit into from
Nov 18, 2019

Conversation

mingwho
Copy link
Contributor

@mingwho mingwho commented Nov 11, 2019

With this change, whoever using the library can pass their own subscriber options.
For example we can pass some of the following options

const subscriberOptions = {
  ackDeadline: 10,
  batching: {
    // https://googleapis.github.io/gax-nodejs/CallSettings.html}
    callOptions: {...},
    maxMessages: 3000,
    maxMilliseconds: 100
  },
  flowControl: {
    allowExcessMessages: true,
    maxBytes: os.freemem() * 0.2,
    maxExtension: Infinity,
    maxMessages: 100
  },
  streamingOptions: {
    highWaterMark: 0,
    maxStreams: 5,
    timeout: 300000
  }
};
subscribe(topic, subscription, onMessage, subscriberOptions)

If subscriberOptions is not provided, we will use default options. If subscriberOptions is {} or [] or "" or null or undefined, or anything that the Pubsub API can't understand, it will use default options.

Documentation is here: https://cloud.google.com/pubsub/docs/pull
Related comments on pubsub subscriber options here: googleapis/nodejs-pubsub#696

});
};

const subscription = pubsub.subscription(subscription_name, options || defaultOptions);
Copy link

Choose a reason for hiding this comment

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

An empty object evaluates to true in JS. So, if options is an empty object {}, we will end up passing it to the subscription function instead of the defaultOptions, is that what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can discuss what to do with this type of input. If the options is an empty object {}, the Pubsub library will use it's own default options, instead of the defaultOptions in this wrapper. This is not expected behavior. Maybe we can have a check to see (if subscriberOptions.ackDeadline || subscriberOptions.batching || subscriberOptions.flowControl || subscriberOptions.streamingOptions), then we use the subscriberOptions. Otherwise, use defaultOptions. What do you think?

Copy link

Choose a reason for hiding this comment

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

If the Pubsub library will use it's default options when the passed options is an empty {}, then I think that's okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok perfect 👍

@mingwho mingwho merged commit 22d3409 into master Nov 18, 2019
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.

None yet

4 participants