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

[ArchBoard] Rename dequeueMessages to receiveMessages, enqueueMessage to sendMessages. Return with the suffix item #4457

Closed
sima-zhu opened this issue Jul 17, 2019 · 11 comments
Assignees
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)

Comments

@sima-zhu
Copy link
Contributor

No description provided.

@sima-zhu sima-zhu self-assigned this Jul 17, 2019
@sima-zhu sima-zhu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) and removed triage labels Jul 17, 2019
@tg-msft
Copy link
Member

tg-msft commented Aug 12, 2019

The proposal is to change DequeueMessages to GetMessages or possibly ReceiveMessages (which aligns more with EventHubs)?

Would there be a similar change from EnqueueMessage to AddMessage?

@sima-zhu
Copy link
Contributor Author

sima-zhu commented Aug 12, 2019

@rickle-msft
Xstore team suggested to have a more general strategy for all get/set APIs
(e.g. enqueue/dequeue, get/setProperties, get/setAccessPolicies)

@rakshith91
Copy link

Note: This is called as recieve_messages in Python.

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Aug 12, 2019

In JS, I'm inclined to go with some variant of "receive messages" instead of "dequeue" similar to servicebus and eventhubs.
Need to discuss with Storage JS team.

@kurtzeborn
Copy link
Member

The naming in the REST endpoint is "getMessages" so that aligns with this suggestion. XStore team agrees with using getMessages instead of dequeueMessages.

Concern: it might be confusing/unexpected to developers that this makes a REST call if it's named getMessages (which they might assume is only accessing local data).

@JonathanGiles, what do you think about this?

@JonathanGiles
Copy link
Member

The Java team is trending towards a naming convention where local operations drop any prefix, e.g. 'get', 'is', 'set' will not be present. Remote operations on the service client class would be the only place where prefixes would be used, and they would continue to be named as appropriate for the operation - in this case 'getMessages'. Even if we don't make this change, the proposal to use 'get' for a remote operation is fine with me.

@rickle-msft
Copy link
Contributor

I'm fine with get/set for remote operations if local operations don't have a prefix (which is a change from what Storage currently has iirc).

If local operations do also use get/set, then piggy-backing on what Scott mentioned, my concern with remote operations sharing get/set with local operations is that until now, our remote operations returned Response whereas now many of them return just T. So previously it should have been clear based on the return type wether getX was remote or local, but with just T, it really only furthers the confusion.

@alzimmermsft
Copy link
Member

@tg-msft @rickle-msft @rakshith91 @HarshaNalluru

Have we reached an agreement on whether we want to move forward with this renaming? Otherwise I propose we close on this issue.

@tg-msft
Copy link
Member

tg-msft commented Sep 20, 2019

We're about to start the next major round of API reviews. Let's all add this suggestion in the QueueClient APIs and let everyone hash it out there.

@alzimmermsft alzimmermsft added the blocking-release Blocks release label Oct 9, 2019
@joshfree joshfree changed the title Bring up the name change for DequeueMessages to the storage group [ArchBoard] Bring up the name change for DequeueMessages to the storage group Oct 10, 2019
@sima-zhu
Copy link
Contributor Author

Working on it.

@joshfree
Copy link
Member

@sima-zhu can you update the title / issue content with the actual change now being tracked (given this issue is now 3 months old and contains a couple variations of the evolved thinking in API naming here)

@sima-zhu sima-zhu changed the title [ArchBoard] Bring up the name change for DequeueMessages to the storage group [ArchBoard] Rename dequeueMessages to getMessages Oct 15, 2019
@sima-zhu sima-zhu changed the title [ArchBoard] Rename dequeueMessages to getMessages [ArchBoard] Rename dequeueMessages to receiveMessages, enqueueMessage to sendMessages. Return with the sufix item Oct 15, 2019
@sima-zhu sima-zhu changed the title [ArchBoard] Rename dequeueMessages to receiveMessages, enqueueMessage to sendMessages. Return with the sufix item [ArchBoard] Rename dequeueMessages to receiveMessages, enqueueMessage to sendMessages. Return with the suffix item Oct 16, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

No branches or pull requests

9 participants