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 Backend Implementation using Stow #189

Merged
merged 22 commits into from
Feb 25, 2022

Conversation

Aayyush
Copy link

@Aayyush Aayyush commented Feb 18, 2022

This PR uses stow to create a generic storage backend that can read and write from s3, azure blob storage, local disk storage etc.

Copy link

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

Good job! Took a first pass

server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/job_store.go Outdated Show resolved Hide resolved
server/core/config/raw/jobs.go Outdated Show resolved Hide resolved
server/core/config/raw/jobs.go Outdated Show resolved Hide resolved
@@ -50,8 +70,57 @@ type StorageBackend struct {
S3 *S3
}

func (s *StorageBackend) GetConfigMap() stow.ConfigMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this struct with the same case statements in every method?

Sounds like this should be an interface with different implementations for each backend.

Copy link
Author

Choose a reason for hiding this comment

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

Yup, I have created a BackendConfigurer interface which is implemented by all configured backends to return the config to setup stow.

server/jobs/job_store.go Outdated Show resolved Hide resolved
delete(j.jobs, jobID)
} else if !errors.Is(err, &StorageBackendNotConfigured{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You wouldn't need this error check if you were using an interface with different implementations.

Copy link
Author

Choose a reason for hiding this comment

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

Decided to use a two value return statement in the Write() method. So, we will no longer have to look for custom errors to decide if we want to delete the job from memory.

server/jobs/storage_backend.go Outdated Show resolved Hide resolved
}

containerFound = true
_, err = container.Put(key, reader, 100, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is 100?

Copy link
Author

Choose a reason for hiding this comment

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

It's the size of the reader according to the documentation. I've updated the GetReader() method to return the size if the reader which is passed into this method call.

server/jobs/storage_backend.go Outdated Show resolved Hide resolved
Copy link

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

Left couple comments

server/jobs/job_store.go Outdated Show resolved Hide resolved
server/jobs/job_store.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
server/jobs/storage_backend.go Outdated Show resolved Hide resolved
Copy link

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

💯

@Aayyush Aayyush merged commit 2f81128 into release-v0.17.3-lyft.1 Feb 25, 2022
@Aayyush Aayyush deleted the aay/jobs-write branch February 25, 2022 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants