[Service Bus] deadletter bug fixes and backup message settlement for track 1 #8406
Conversation
HarshaNalluru
commented
Apr 16, 2020
•
HarshaNalluru
commented
- Deadletter bug fixes porting from track 2
- Backup message settlement for track 1 (similar to track 2 - [Service Bus] Backup Message Settlement with managementLink - track 2 #8340)
- Added tests
HarshaNalluru
requested review from
bterlson,
chradek,
ramya-rao-a and
richardpark-msft
as code owners
|
The changes look good, but is it possible to port some of the tests from track2 over as well? |
Porting all of the tests.. ongoing. |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| operation: DispositionStatus, | ||
| options?: DispositionStatusOptions | ||
| ): Promise<void> { | ||
| const dispositionType = getDispositionType(operation); |
There was a problem hiding this comment.
Do we really need this helper method? Can we not include the disposition type as an input parameter?
The helper opens up the possibility of the return type being undefined and then we are forced to handle that with using the !
Side note: In Track 2, I want to investigate why we need both enums DispositionStatus and DispositionType.
There was a problem hiding this comment.
Delivery object expects DispositionType whereas updateDispositionStatus expects DispositionStatus.
There was a problem hiding this comment.
Actually, DispositionType is getting used only in error messages or in if statements :)
Anyway, my feedback here was to avoid having the getDispositionType helper and plumb through the DispositionType just like you are doing with DispositionStatus for the reasons I mentioned above. But I dont feel too strongly about it anymore
There was a problem hiding this comment.
Sure, I totally agree.
Also unrelated.. I just realized that I should also verify the message lock renewal along with backup settlement(and add some tests).
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
…rshaNalluru/azure-sdk-for-js into harshan/sb/issue/4737-track-1
…zure-sdk-for-js into harshan/sb/issue/4737-track-1
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-Authored-By: Ramya Rao <ramya.rao.a@outlook.com>
| senderType: TestClientType, | ||
| receiverType: TestClientType | ||
| ): Promise<void> { | ||
| const useSessions = receiverType > 5 && receiverType < 12 ? true : false; |
There was a problem hiding this comment.
Well this check scares me, and actually looks like a case where a bit flag could be useful. Since that might mean making a lot of changes to this PR, could you add a comment to TestClientType that states that tests depend on this enum not changing?
There was a problem hiding this comment.
I will add a comment.
Co-Authored-By: chradek <51000525+chradek@users.noreply.github.com>
…rshaNalluru/azure-sdk-for-js into harshan/sb/issue/4737-track-1
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
