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

[storage][stg74] share lease #11450

Closed

Conversation

ljian3377
Copy link
Member

No description provided.

@ghost ghost added Event Hubs Storage Storage Service (Queues, Blobs, Files) labels Sep 24, 2020
@ljian3377 ljian3377 changed the base branch from master to storage/stg74base September 24, 2020 04:01
@jiacfan
Copy link
Member

jiacfan commented Sep 24, 2020

  assert.fail("acquireLease should fail for an invalid duration: -2");

This doesn't match the current value 20


Refers to: sdk/storage/storage-file-share/test/leaseclient.spec.ts:104 in c5d9249. [](commit_id = c5d9249, deletion_comment = False)

@@ -96,7 +96,8 @@ describe("LeaseClient", () => {
});

it("invalid duration for acquireLease", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

May worth add more cases to cover newly added functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in follow-up PR.

@XiaoningLiu
Copy link
Member

public getShareLeaseClient(proposeLeaseId?: string) {

Why fileClient can get ShareLeaseClient? It just sounds like we can get containerClient from blobClient.


Refers to: sdk/storage/storage-file-share/src/Clients.ts:1340 in c5d9249. [](commit_id = c5d9249, deletion_comment = False)

@XiaoningLiu
Copy link
Member

XiaoningLiu commented Sep 24, 2020

export class ShareLeaseClient {

The naming is confusing if ShareLeaseClient can actually manage lease for share or file.

Ideally we should rename it to LeaseClient, but that will be a breaking change.

How about, we rename it to LeaseClient and expose a alias of "ShareLeaseClient" to be deprecated in next major release.


Refers to: sdk/storage/storage-file-share/src/ShareFileClient.ts:5700 in c5d9249. [](commit_id = c5d9249, deletion_comment = False)

@ljian3377
Copy link
Member Author

ljian3377 commented Sep 24, 2020

export class ShareLeaseClient {

The naming is confusing if ShareLeaseClient can actually manage lease for share or file.

Ideally we should rename it to LeaseClient, but that will be a breaking change.

How about, we rename it to LeaseClient and expose a alias of "ShareLeaseClient" to be deprecated in next major release.

Refers to: sdk/storage/storage-file-share/src/ShareFileClient.ts:5700 in c5d9249. [](commit_id = c5d9249, deletion_comment = False)

True. It's poorly named by me when doing lease for file. But we previously name FileClient as ShareFileClient, DirectoryClient as ShareDirectoryClient. And we name it BlobLeaseClient in Blob. So I figure we need some prefix 😢

@ljian3377
Copy link
Member Author

Changes in this PR was already merged to stg74base by #11455.

@ljian3377 ljian3377 closed this Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants