-
Notifications
You must be signed in to change notification settings - Fork 493
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
Remove telemetry batch operation over MQTT #3000
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Comments inline.
Co-authored-by: David R. Williamson <drwill@microsoft.com>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
// is the recommended pattern for sending large numbers of messages over an asynchronous | ||
// protocol like MQTT | ||
await Task.WhenAll(messages.Select(x => SendTelemetryAsync(x, cancellationToken))).ConfigureAwait(false); | ||
throw new InvalidOperationException("Batch telemetry is supported only over AMQP."); |
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 feel like the MQTT transport layer shouldn't know about the other transports. A more cohesive message would be that it is not supported over MQTT. Thoughts?
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 see your point. I think we'll still need to guide users down the correct path.
How about:
throw new InvalidOperationException("This operation is not supported over this protocol. Please refer to the doc comments for additional details.");
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
No description provided.