Skip to content

delete pull-related methods from PubSub #1487

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 22, 2016
Merged

delete pull-related methods from PubSub #1487

merged 7 commits into from
Dec 22, 2016

Conversation

pongad
Copy link
Contributor

@pongad pongad commented Dec 21, 2016

Instead, provide a way to create the Subscriber object.

cc @davidtorres

@pongad
delete pull-related methods from PubSub
135a4b2 
Instead, provide a way to create the Subscriber object.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2016
davidtorres
davidtorres reviewed Dec 21, 2016
View reviewed changes
google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/PubSubImpl.java
return modifyAckDeadlineAsync(subscription, 0, TimeUnit.SECONDS, ackIds);
}

@Override
public void modifyAckDeadline(String subscription, int deadline, TimeUnit unit, String ackId,

This comment was marked as spam.

Sign in to view

pongad
pongad commented Dec 21, 2016
View reviewed changes
google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/PubSubImpl.java
.maxQueuedCallbacks(MAX_QUEUED_CALLBACKS.getInteger(optionMap))
.executorFactory(EXECUTOR_FACTORY.getExecutorFactory(optionMap))
public Subscriber subscriber(SubscriptionInfo subscription, Subscriber.MessageReceiver receiver) {
// TODO(pongad): Provide a way to pass in the rest of the options.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9df40ba on pongad:del-pull into ** on GoogleCloudPlatform:pubsub-hp**.

@pongad
Copy link
Contributor Author

pongad commented Dec 21, 2016

@davidtorres PTAL

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2239a50 on pongad:del-pull into ** on GoogleCloudPlatform:pubsub-hp**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2239a50 on pongad:del-pull into ** on GoogleCloudPlatform:pubsub-hp**.

@garrettjonesgoogle
Copy link
Member

A couple high-level things:

  • We need to write down in our TO DO list to write new samples that replace the deleted ones here (in spirit)
  • Does our new code have the same level of test coverage as the code that's being deleted?
  • We might want to write down some migration details for anyone using the library currently. Could you draft up a list of equivalents to each method on the main surface that you're deleting?

@pongad
Copy link
Contributor Author

pongad commented Dec 22, 2016

@garrettjonesgoogle

  • I made an issue for the TODO list: Tracking issue for PubSub high-perf client integration #1493
  • According to https://coveralls.io/builds/9371615, we have 80%+ on most classes. PollingSubscriberConnection is an exception at 76%. The missed lines are for expoenential backoff. They can be tested relatively easily I think.
  • The migration document is at google-cloud-pubsub/HP-MIGRATION.md, added to the last commit. There isn't that much to document, since the new surface is pretty small.

@garrettjonesgoogle
Copy link
Member

Add a TODO to settle the issue of executors & channels. Otherwise LGTM.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6cd3804 on pongad:del-pull into ** on GoogleCloudPlatform:pubsub-hp**.

@pongad pongad mentioned this pull request Dec 22, 2016
Closed
6 tasks
@pongad pongad merged commit 994479f into googleapis:pubsub-hp Dec 22, 2016
@pongad pongad deleted the del-pull branch December 22, 2016 01:17
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 46bf12f on pongad:del-pull into ** on GoogleCloudPlatform:pubsub-hp**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtorres davidtorres davidtorres left review comments

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle left review comments

Assignees

@garrettjonesgoogle garrettjonesgoogle

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pongad @coveralls @garrettjonesgoogle @davidtorres @googlebot