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

[Service Bus] Better errors for settling deferred msg in ReceiveAndDelete mode #10396

Merged
merged 4 commits into from
Aug 5, 2020

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Aug 2, 2020

As reported in #10395, we have inconsistent errors being returned to the user when settling a deferred message in Receive And Delete mode.

There are 2 different root causes here

  • When trying to settle, the client fails to identify that the message was received in ReceiveAndDelete mode
  • The service sets a lock token on messages received in ReceiveAndDelete mode (which it should not) if the entity in question is partitioned.

The solution being proposed in this PR is

  • The client sets the lockToken on the message to undefined if the message was received in ReceiveAndDelete mode.
  • The absence of the lockToken is then taken to be the indicator of the ReceiveAndDelete mode when settling, and a consistent error is thrown

@ghost ghost added the Service Bus label Aug 2, 2020
@ramya-rao-a ramya-rao-a force-pushed the deferred-delete-mode branch from a19cacc to fe6f7d7 Compare August 2, 2020 05:51
@ramya-rao-a ramya-rao-a marked this pull request as ready for review August 2, 2020 17:31
Comment on lines +509 to +512
// we have to force this cast - the type system doesn't allow this if you've chosen receiveAndDelete
// as your lock mode.

// have to cast it - the type system doesn't allow us to call into this method otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Repetitive?

Suggested change
// we have to force this cast - the type system doesn't allow this if you've chosen receiveAndDelete
// as your lock mode.
// have to cast it - the type system doesn't allow us to call into this method otherwise.
// we have to force this cast - the type system doesn't allow us to call into this method otherwise.

Copy link
Contributor Author

@ramya-rao-a ramya-rao-a Aug 3, 2020

Choose a reason for hiding this comment

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

no idea, I copied the entire test set from the ones we have for "normal" messages in this file :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the problem is that the Receiver isn't being typed more specifically.

We should fix this at some point to allow the receiver to be scoped properly but I think (grossly) this is the best we can do for now.

Copy link
Member

@HarshaNalluru HarshaNalluru left a comment

Choose a reason for hiding this comment

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

LGTM!

) {
Object.assign(this, fromAmqpMessage(msg, delivery, shouldReorderLockToken));
if (receiveMode === ReceiveMode.receiveAndDelete) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just pop a comment in here saying this is a workaround (because eventually we can get rid of this, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in 6fec199

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks fine to me, just add a comment so we know why we unset the lock token.

should.equal(errorWasThrown, true, "Error thrown flag must be true");
}

it(noSessionTestClientType + ": complete() throws error", async function(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

would it be simpler to just do the [].forEach(() => {}) trick instead?

Like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants