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

[IA-2355] support maxRetry #372

Merged
merged 5 commits into from
Nov 17, 2020
Merged

[IA-2355] support maxRetry #372

merged 5 commits into from
Nov 17, 2020

Conversation

Qi77Qi
Copy link
Collaborator

@Qi77Qi Qi77Qi commented Nov 16, 2020

support optional subscriber name and optional deadLetterPolicy

Tested in console:

qi | INFO: Subscriber Successfully received data: "\"ooo\""
attributes {
  key: "googclient_deliveryattempt"
  value: "5"
}
message_id: "501898938879014"
publish_time {
  seconds: 1605568147
  nanos: 653000000
}
.

After 5 retries, msg is forwarded to dead letter topic that I configed, I see the nacked message as expected.

PR checklist

  • For each library you've modified here, decide whether it requires a major, minor, or no version bump. (Click here for guidance)

If you're doing a major or minor version bump to any libraries:

  • Bump the version in project/Settings.scala createVersion()
  • Update CHANGELOG.md for those libraries
  • I promise I used @deprecated instead of deleting code where possible

In all cases:

  • Replace the appropriate version hashes in README.md and the CHANGELOG.md for any libs you changed with TRAVIS-REPLACE-ME to auto-update the version string
  • Get two thumbsworth of PR review
  • Verify all tests go green (CI and coverage tests)
  • Squash commits and merge to develop
  • Delete branch after merge

## 0.16

Changed:
- Add `subscriptionName: Option[ProjectSubscriptionName]` and `deadLetterPolicy: Option[SubscriberDeadLetterPolicy]` to `SubscriberConfig`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious - since these are optional could it be a non-breaking change if they default to None?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah yea, I guess (I'm not entirely sure by providing default is backward compatible since I vaguely remember I had to fix things after a PR that added optional param with default...)...I tend to not giving default just so caller have to think about these options. But I'm happy to give default here..also This has more points on why providing defaults can be bad

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't love defaults either, was just curious. I don't really have a strong preference, will thumb as is :)

}
traceId = Option(message.getAttributesMap.get("traceId")).map(s => TraceId(s))
} yield Event(msg, traceId, message.getPublishTime, consumer)

val loggingCtx = Map(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this context not useful to log, or is it logged in another way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when we log the PubsubMessage in the message, these context are already being logged. So it's a bit duplicate

@Qi77Qi Qi77Qi merged commit a002ea3 into develop Nov 17, 2020
@Qi77Qi Qi77Qi deleted the maxAttempts branch November 17, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants