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

Update lakectl abuse to use set/link physical address #7743

Merged
merged 3 commits into from
May 9, 2024

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented May 8, 2024

Closes #7740

Change Description

Background

Modify lakectl abuse random-write so that it will reflect the behavior of lakeFS

Bug Fix

Added some bug fixes and tests to the abuse commands

Testing Details

Added esti tests

@N-o-Z N-o-Z added bug Something isn't working area/lakectl Issues related to lakeFS' command line interface (lakectl) exclude-changelog PR description should not be included in next release changelog labels May 8, 2024
@N-o-Z N-o-Z requested review from itaiad200 and a team May 8, 2024 10:25
@N-o-Z N-o-Z self-assigned this May 8, 2024
Copy link

github-actions bot commented May 8, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

@N-o-Z N-o-Z force-pushed the task/lakectl-abuse-use-link-physical-addr-7740 branch from 7b785f0 to 5ae1894 Compare May 8, 2024 10:43
@N-o-Z N-o-Z changed the title Task/lakectl abuse use link physical addr 7740 Update lakectl abuse to use set/link physical address May 8, 2024
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests!

}
})
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a simple comment about the fact that you're linking without uploading to the object store?

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Don't understand the driver code, sorry. The whole objects upload run command part is weird.

cmd/lakectl/cmd/abuse_commit.go Show resolved Hide resolved
}
for _, tt := range tests {
t.Run(tt.Cmd, func(t *testing.T) {
RunCmdAndVerifyContainsText(t, Lakectl()+" abuse "+tt.Cmd+" lakefs://"+repoName+"/"+mainBranch+" --amount "+strconv.Itoa(tt.Amount)+" "+tt.AdditionalArgs, false, "errors: 0", map[string]string{})
Copy link
Contributor

Choose a reason for hiding this comment

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

This string may be easier to read if generated from a template or using fmt.Sprintf.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

const totalObjects = 5
for i := 0; i < totalObjects; i++ {
vars["FILE_PATH"] = fmt.Sprintf("data/ro/ro_1k.%d", i)
fromFile = fromFile + vars["FILE_PATH"] + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this loop at all.

  1. Each iteration appends an entire new line to fromFile! How does that run?
  2. What writes these 5 objects? Do they need to be added to the repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The commands read a file from the provided param and reads line by lines keys from the file
  2. What writes these 5 objects is line 876 (now 875), we use an existing test file in our repo for the physical object

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks! The code is a bit confusing. I would probably find it easier to read if you wrote the filename to the abuse-read file immediately, rather than build a string in-memory and then write it on l. 879.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree but this requires us to handle stdin input in the lakectl tests... 😬

@N-o-Z N-o-Z requested a review from arielshaqed May 9, 2024 08:31
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

THANKS!

const totalObjects = 5
for i := 0; i < totalObjects; i++ {
vars["FILE_PATH"] = fmt.Sprintf("data/ro/ro_1k.%d", i)
fromFile = fromFile + vars["FILE_PATH"] + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks! The code is a bit confusing. I would probably find it easier to read if you wrote the filename to the abuse-read file immediately, rather than build a string in-memory and then write it on l. 879.

@N-o-Z N-o-Z merged commit 1ff4f50 into master May 9, 2024
35 checks passed
@N-o-Z N-o-Z deleted the task/lakectl-abuse-use-link-physical-addr-7740 branch May 9, 2024 09:45
emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024
* Update lakectl abuse to use set/link physical address

* CR Fixes

* Fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lakectl Issues related to lakeFS' command line interface (lakectl) bug Something isn't working exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify lakectl abuse "write" to use get/link physical address instead of stage object
3 participants