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

[Perf Tests] [AMQP] Test naming consistency update #26674

Merged

Conversation

HarshaNalluru
Copy link
Member

For consistent naming across languages

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Class: SendTest
Arguments:
- --event-size 1024 --batch-size 100 --parallel 64

- Test: subscribe
- Test: receive-events
Copy link
Member

Choose a reason for hiding this comment

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

Nit: On second thought, I feel like we should also have the -batch suffix here for a bit more clarity and consistency, especially since looking at the SB tests, we test both receive in batch/or non-batch form. In Python we have EH perf-tests for receive in batchs/non-batches, as well. Not a blocker, and ultimately doesn't matter too much, but WDYT?

Copy link
Member Author

@HarshaNalluru HarshaNalluru Aug 7, 2023

Choose a reason for hiding this comment

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

receive-queue-messages-batch in SB is different from receive-events here.

// receive-queue-messages-batch in SB
messages = await receiver.receiveMessages(...)
// (= Batch)

// receive-queue-messages in SB
// Alternative name: process-queue-messages
    receiver.subscribe(
      {
        processMessage: async (_message: ServiceBusReceivedMessage) => {
          .....
        },
        processError: async (args: ProcessErrorArgs) => {
          ....
        },
      }
    );
// receive-events in EH
// Alternative name: process-events-batch
    receiver.subscribe(
      {
        processEvents: async (events: ReceivedEventData[], _context: PartitionContext) => {
          // ..... events (= Batch)
        },
        processError: async (error: Error | MessagingError, _context: PartitionContext) => { 
          .....
        },
      }
    );

Adding batch to EH streaming might be misleading in the context of EH vs SB.


If you want to put "batch" in the name, I would prefer..

  1. receive-queue-messages-batch in SB
  2. process-queue-messages in SB
  3. process-events-batch in EH

Copy link
Member

Choose a reason for hiding this comment

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

That works for me :)

@HarshaNalluru HarshaNalluru merged commit 414d5ad into Azure:main Aug 8, 2023
24 checks passed
@HarshaNalluru HarshaNalluru deleted the harshan/perf-amqp-consistency branch August 8, 2023 21:34
dgetu pushed a commit that referenced this pull request Sep 6, 2023
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.

6 participants