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.

Sign up for GitHub

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

Jump to bottom

Optionally set mtime in linkPhysicalAddress #8338

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Optionally set mtime in linkPhysicalAddress #8338

merged 2 commits into from
Nov 5, 2024

Conversation

arielshaqed
Copy link
Contributor

@arielshaqed arielshaqed commented Nov 3, 2024 •
edited
Loading

Closes #8337. Will be used in #8339.

@arielshaqed
Add optional `mtime' to linkPhysicalAddress
183a03c 
- Ignored by old servers
  - Tested: when a new client sends mtime to an old server, the old server
	ignores it.
- Sets object mtime
  - This is like stageObject, so (almost) already available
@arielshaqed
Use optional `mtime' in linkPhysicalAddress
8490de7
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2024 •
edited
Loading

♻️ PR Preview 8490de7 has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@arielshaqed arielshaqed added area/cataloger Improvements or additions to the cataloger area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Nov 3, 2024
@arielshaqed arielshaqed requested a review from N-o-Z November 3, 2024 14:49
@arielshaqed
Copy link
Contributor Author

Will submit with approval from any reviewer unless otherwise requested.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 3, 2024

E2E Test Results - Quickstart

11 passed

itaigilo
itaigilo reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@itaigilo itaigilo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The fix itself LGTM -
Though I don't have enough context about what it's trying to solve.

Following here.

N-o-Z
N-o-Z approved these changes Nov 4, 2024
View reviewed changes
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

LGTM!
I assume there's a follow up issue for LakeFSFS?

@arielshaqed
Copy link
Contributor Author

LGTM! I assume there's a follow up issue for LakeFSFS?

Thanks! Yup, forgot to link to #8339.

@arielshaqed arielshaqed merged commit 5128ee9 into master Nov 5, 2024
47 of 48 checks passed
@arielshaqed arielshaqed deleted the feature/lakefs-optionally-set-mtime-in-link-phys branch November 5, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itaigilo itaigilo itaigilo left review comments

@N-o-Z N-o-Z N-o-Z approved these changes

@itaiad200 itaiad200 Awaiting requested review from itaiad200

Assignees
No one assigned
Labels
area/API Improvements or additions to the API area/cataloger Improvements or additions to the cataloger include-changelog PR description should be included in next release changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting mtime in linkPhysicalAddress
3 participants
@arielshaqed @N-o-Z @itaigilo