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

sleep() and clock() should return az_result #1226

Closed
antkmsft opened this issue Sep 8, 2020 · 6 comments · Fixed by #1256
Closed

sleep() and clock() should return az_result #1226

antkmsft opened this issue Sep 8, 2020 · 6 comments · Fixed by #1256
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@antkmsft
Copy link
Member

antkmsft commented Sep 8, 2020

We have only two functions in the PAL layer: void sleep() and int64 clock(). We should change them to az_result sleep() and az_result clock(int64* out), so that they would return AZ_ERROR_DEPENDENCY_NOT_PROVIDED, so the code catches it early and exits without possibly attempting to work, but working in a strange way. So, if your code ends up relying on these functions, and it fails, you'll get a clear error code telling about the problem, and sooner.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 8, 2020
@RickWinter RickWinter added Azure.Core Client This issue points to a problem in the data-plane of the library. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Sep 8, 2020
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 8, 2020
@RickWinter RickWinter added the help wanted This issue is tracking work for which community contributions would be welcomed and appreciated label Sep 8, 2020
@ahsonkhan
Copy link
Contributor

ahsonkhan commented Sep 8, 2020

That might be optimizing the API design too much for the "failure" condition. Having the clock not return the current time hurts usability. Similarly, having sleep return something might be surprising to users.

@JeffreyRichter - what are your thoughts on this?

@antkmsft antkmsft added this to the [2020] October milestone Sep 8, 2020
@RickWinter
Copy link
Member

RickWinter commented Sep 8, 2020

@ahsonkhan It feels reasonable to adopt this design pattern. I would expect this pattern on all pal APIs so we should review carefully before making the change

@JeffreyRichter
Copy link
Member

The only code "using"/calling these functions is our code so I don't care much about the usability. These function are NOT part of our SDK's public API and so customer can't call them directly but they have no need to because they had to provide them to us in the first place.

@antkmsft
Copy link
Member Author

antkmsft commented Sep 8, 2020

Another possible solution proposal by Jeff: put preconditions that always fail (in az_noplatform implementation)

From myself: why don't we do the same in az_nohttp? And I think we should not be doing this in az_nohttp, because you run a sample and suddenly you have to learn preconditions, not the best possible experience, although it is not that bad. Maybe platform and http are different in this aspect - what is good for az_noplatform, doesn't work from the usability standpoint in az_nohttp.

@ahsonkhan
Copy link
Contributor

These function are NOT part of our SDK's public API and so customer can't call them directly but they have no need to because they had to provide them to us in the first place.

It is the interface that we define, that the user has to implement. I would consider that as public API.

@JeffreyRichter
Copy link
Member

Well, OK, yes the function signatures are documented (if you want to call this public) but I do not think of the functions as being part of our public SDK surface area that consumers of our SDK would call directly. The PAL functions implementations are just to get the SDK working for US.

@ahsonkhan ahsonkhan removed the help wanted This issue is tracking work for which community contributions would be welcomed and appreciated label Sep 10, 2020
@RickWinter RickWinter added design-discussion An area of design currently under discussion and open to team and community feedback. and removed Design labels Aug 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
4 participants