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

Obj store through sidecar #4741

Merged
merged 6 commits into from
Mar 26, 2020
Merged

Obj store through sidecar #4741

merged 6 commits into from
Mar 26, 2020

Conversation

jdoliner
Copy link
Member

No description provided.

@jdoliner
Fix make proto.
bf30db1
@jdoliner
Route object store access through sidecar.
03326a8 
This gets us around issue that may arise from user containers with weird
network settings.
@jdoliner
Merge branch 'master' into obj_store_through_sidecar
57c6e46
@netlify
Copy link

netlify bot commented Mar 24, 2020 •
edited
Loading

Your website preview is ready! Hooray! 🎉

Built with commit 6400f8d

https://deploy-preview-4741--pachyderm-docs.netlify.com

@jdoliner jdoliner requested a review from Tryneus March 24, 2020 20:51
Tryneus
Tryneus suggested changes Mar 24, 2020
View reviewed changes
Copy link
Contributor

@Tryneus Tryneus left a comment

Choose a reason for hiding this comment

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

I'd like to see some tests for this if we're going to merge this into v1.10.x, otherwise LGTM

src/client/pfs/pfs.proto Outdated
@@ -664,6 +664,15 @@ message Objects {
repeated Object objects = 1;
}

message PutObjRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the names here could maybe be less ambiguous - Obj and Object result in very different behaviors, which isn't immediately obvious. Maybe like PutObjectRaw/GetObjectRaw or Direct or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point I can do that.

@jdoliner
Copy link
Member Author

What did you have in mind for tests of this behavior specifically. Other than trying to come up with containers that reproduce the original issue, which I'm kind of unsure how to do, it seems like our existing pipeline tests are basically the same as whatever tests I would write.

@Tryneus
Copy link
Contributor

Tryneus commented Mar 24, 2020 •
edited
Loading

After a quick offline discussion, I withdraw my objection for testing until such time as we have a better mock object storage client framework for tests.

@jdoliner
Copy link
Member Author

Renames done.

@Tryneus
Copy link
Contributor

Tryneus commented Mar 25, 2020

LGTM

@jdoliner jdoliner merged commit 4e5a67d into master Mar 26, 2020
@echohack echohack deleted the obj_store_through_sidecar branch March 18, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tryneus Tryneus Tryneus approved these changes

Assignees

@Tryneus Tryneus

Labels
1.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdoliner @Tryneus @nitinjainsj