Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

storage: update WalkDir#467

Merged
3pointer merged 10 commits intopingcap:masterfrom
3pointer:update_s3_api
Aug 24, 2020
Merged

storage: update WalkDir#467
3pointer merged 10 commits intopingcap:masterfrom
3pointer:update_s3_api

Conversation

@3pointer
Copy link
Collaborator

@3pointer 3pointer commented Aug 21, 2020

What problem does this PR solve?

resolve #463 (comment)
move dir listCount parameters into context

What is changed and how it works?

update WalkDir api

Check List

Tests

  • No code

Related changes

  • Need to cherry-pick to the release branch

Release Note

  • No release note

glorv
glorv reviewed Aug 21, 2020
View reviewed changes
kennytm
kennytm previously requested changes Aug 21, 2020
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

it is a bad idea to have behavior-changing arguments hidden inside the context.

Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.

@glorv
Copy link
Collaborator

glorv commented Aug 21, 2020

it is a bad idea to have behavior-changing arguments hidden inside the context.

Then shall we add another contextual generic param instead of put them in the context?

@kennytm
Copy link
Collaborator

kennytm commented Aug 21, 2020 •
edited
Loading

we should put these into an options struct but there's no need to make a storage-specific S3Context type.

it's easy to determine the common options between S3, GCS and local storage:

  • the subdirectory path (prefix in S3 and GCS, sub directory in Local)
  • the optimal "page size" (max-keys in S3, maxResults in GCS, ignored in Local)
  • a nullable "page token" (continuation-token in S3, pageToken in GCS, ignored in Local)

the return value of WalkDir should also include the next "page token" (NextContinuationToken in S3, nextPageToken in GCS, always nil in Local).

(but perhaps we could just automatically issue N requests to get rid of both page size and page token)

kennytm
kennytm reviewed Aug 24, 2020
View reviewed changes
@kennytm kennytm dismissed their stale review August 24, 2020 07:16

no longer hiding parameters in Context

kennytm
kennytm reviewed Aug 24, 2020
View reviewed changes
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM, though ideally we should get rid of ListCount.

@kennytm kennytm added the status/LGT1 LGTM1 label Aug 24, 2020
@glorv
Copy link
Collaborator

glorv commented Aug 24, 2020

LGTM

@ti-srebot
Copy link
Contributor

ti-srebot commented Aug 24, 2020

@glorv,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: tools(slack).

@glorv glorv added status/LGT2 LGTM2 and removed status/LGT1 LGTM1 labels Aug 24, 2020
overvenus
overvenus approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added status/LGT1 LGTM1 and removed status/LGT2 LGTM2 labels Aug 24, 2020
@3pointer 3pointer merged commit 1fd8463 into pingcap:master Aug 24, 2020
@ti-srebot
Copy link
Contributor

ti-srebot commented Aug 24, 2020

cherry pick to release-4.0 failed

3pointer added a commit to 3pointer/br that referenced this pull request Aug 24, 2020
@3pointer @kennytm
storage: update WalkDir (pingcap#467)
1dfc9e7 
* storage: update WalkDir



Co-authored-by: kennytm <kennytm@gmail.com>
@3pointer 3pointer mentioned this pull request Aug 24, 2020
Merged
kennytm added a commit that referenced this pull request Sep 1, 2020
@3pointer @kennytm @YuJuncen
storage: update WalkDir (#467) (#469)
e97424e 
* storage: update WalkDir



Co-authored-by: kennytm <kennytm@gmail.com>

Co-authored-by: kennytm <kennytm@gmail.com>
Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@kennytm kennytm kennytm left review comments

@glorv glorv glorv left review comments

@overvenus overvenus overvenus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-cherry-pick-release-4.0 status/LGT1 LGTM1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@3pointer @glorv @kennytm @ti-srebot @overvenus