-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: Track 2 Preview 1 API surface #7196
Service Bus: Track 2 Preview 1 API surface #7196
Conversation
438c52f
to
04f0de2
Compare
Some things that are missing - some settings (iteration specifically) aren't yet plumbed thorugh so we can't configure timeouts or max wait times properly.
* receiveBatch() * options to the main methods * peek (diagnostics.peek)
Expose autcomplete options
* Removing some of the temporary naming that was in place as we removed older classe
- Remove the need for QueueAuths or SubscriptionAuths
- rename iterateMessages to getMessageIterator
get sessionId(): string | undefined; | ||
get sessionLockedUntilUtc(): Date | undefined; | ||
sessionId: string | undefined; | ||
sessionLockedUntilUtc: Date | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sessionId
and sessionLockedUntilUtc
previously were not settable by the user. Now they are. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We should make them read-only.
service-bus.api.md
Outdated
[key: string]: any; | ||
}): Promise<void>; | ||
complete(): Promise<void>; | ||
deadLetter(options?: DeadLetterOptions & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options -> propertiesToModify ?
service-bus.api.md
Outdated
// @public | ||
export interface CreateBatchOptions extends OperationOptions { | ||
maxSizeInBytes?: number; | ||
retryOptions?: RetryOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do recall that this was added upon my insistence in another PR, but I believe the retry options shouldn't be here.
We do not need to have retryOptions
customizable by the user in every method. At the bare minimum it should appear in the client constructor. As an extra, it may appear in the getSender/getReceiver methods.
service-bus.api.md
Outdated
|
||
export { delay } | ||
|
||
export { Delivery } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delivery
was being exported before as it was part of the service bus message. Since we no longer expose this in the message, we don't need to export it
service-bus.api.md
Outdated
// @public | ||
export interface Receiver<ReceivedMessageT> { | ||
close(): Promise<void>; | ||
diagnostics: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side thought: After recent customer interviews, I wonder if the word diagnostics
may be more understood as diagnostics for the receiver and less as diagnostics for the queue/subscription i.e a user might expect to find methods or properties indicating whether the receiver/connection is open or closed or broken or in the process of reconnecting etc
service-bus.api.md
Outdated
isReceivingMessages(): boolean; | ||
receiveBatch(maxMessages: number, options?: ReceiveBatchOptions): Promise<ReceivedMessageT[]>; | ||
receiveDeferredMessage(sequenceNumber: Long, options?: OperationOptions): Promise<ReceivedMessageT | undefined>; | ||
receiveDeferredMessages(sequenceNumbers: Long[], options?: OperationOptions): Promise<ReceivedMessageT[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the single and multiple form for receiving deferred messages, but not for peek or receiveBatch. What are we using to draw the line between which features get the single & multiple variations and which get just the multiple one?
service-bus.api.md
Outdated
getDeadLetterReceiver(queueName: string, receiveMode: "receiveAndDelete"): Receiver<ReceivedMessage>; | ||
getDeadLetterReceiver(topicName: string, subscriptionName: string, receiveMode: "peekLock"): Receiver<ReceivedMessageWithLock>; | ||
getDeadLetterReceiver(topicName: string, subscriptionName: string, receiveMode: "receiveAndDelete"): Receiver<ReceivedMessage>; | ||
getReceiver(queueName: string, receiveMode: "peekLock"): Receiver<ReceivedMessageWithLock>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way that we can continue to have receive mode as an enum and still get the ReceivedMessageT fun? My concern with using string literals is opening up the chances of spelling mistakes
service-bus.api.md
Outdated
diagnostics: { | ||
peek(maxMessageCount?: number): Promise<ReceivedMessage[]>; | ||
peekBySequenceNumber(fromSequenceNumber: Long, maxMessageCount?: number): Promise<ReceivedMessage[]>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagnostics here appear to be the same as in the Receiver
. Since SessionReceiver
is extending the Receiver
, should this be appearing here at all?
export interface SessionReceiverOptions { | ||
maxSessionAutoRenewLockDurationInSeconds?: number; | ||
sessionId: string | undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface isn't being used anywhere
service-bus.api.md
Outdated
|
||
export { TokenCredential } | ||
|
||
export { TokenType } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is TokenType
being exported?
export class ServiceBusClient { | ||
constructor(connectionString: string, options?: ServiceBusClientOptions); | ||
constructor( | ||
hostName: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostName -> fullyQualifiedNamespace
close(): Promise<void>; | ||
getDeadLetterReceiver( | ||
queueName: string, | ||
receiveMode: "peekLock", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create an issue to discuss the need to make receive mode a mandatory parameter
|
||
// @public | ||
export interface GetReceiverOptions { | ||
retryOptions?: RetryOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The theory that one would want to have retry options for a sender/receiver that is different from the ones set on the client is not a proved one. This should be something we add when we have user feedback for the same. Since this can be additional, we can always add it back.
|
||
// @public | ||
export interface SessionReceiverOptions { | ||
maxSessionAutoRenewLockDurationInSeconds?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
milli seconds and maybe maxAutoRenewLockDurationInMs
or autoRenewLockDurationInMs
export interface SessionReceiverOptions { | ||
maxSessionAutoRenewLockDurationInSeconds?: number; | ||
retryOptions?: RetryOptions; | ||
sessionId: string | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark as optional parameter instead of | undefined
?
options?: GetReceiverOptions | ||
): Receiver<ReceivedMessage>; | ||
getSender(queueOrTopicName: string, options?: GetSenderOptions): Sender; | ||
getSessionReceiver( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have feedback for this to be async. We need issue to talk about getReceiver and getSender to be async as well
constructor(connectionString: string, options?: ServiceBusClientOptions); | ||
constructor( | ||
hostName: string, | ||
tokenCredential: TokenCredential, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tokenCredential
-> credential
export interface ServiceBusMessage { | ||
body: any; | ||
contentType?: string; | ||
correlationId?: string | number | Buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback from @bterlson is that this should support UInt8array as well if it supports Buffer
export interface SessionReceiver< | ||
ReceivedMessageT extends ReceivedMessage | ReceivedMessageWithLock | ||
> extends Receiver<ReceivedMessageT> { | ||
diagnostics: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diagnostics
can give the impression that one can have diagnostic operations on the state of the receiver. If the intention here is to avoid confusion between peek()
the method and peeklock
the mode, then we can experiment with browse
first
import { AmqpMessage } from "@azure/core-amqp"; | ||
import { delay } from "@azure/core-amqp"; | ||
import { Delivery } from "rhea-promise"; | ||
import Long from "long"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need an issue to discuss the long and short of Long
peekBySequenceNumber( | ||
fromSequenceNumber: Long, | ||
maxMessageCount?: number | ||
): Promise<ReceivedMessage[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move the count and the sequence number to options along with operation options
This is the current surface for preview.1.
Some of the more interesting bits: