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

Add support for using the ObjectStore pattern #78

Merged
merged 24 commits into from
Jul 6, 2023
Merged

Conversation

mmmries
Copy link
Owner

@mmmries mmmries commented Jul 1, 2023

Coneptual documentation: https://docs.nats.io/nats-concepts/jetstream/obj_store/obj_walkthrough
ADR: https://github.com/nats-io/nats-architecture-and-design/blob/main/adr/ADR-20.md

Similar to the Key-Value functionality, this is not actually a specific thing built into the nats-server, but a documented pattern for how to store chunked files. This allows for efficient storage/streaming of larger files. I'll be modeling my changes from the ADR notes as well as the time-honored tradition of doing nats-server -js -DV and reverse engineering the logs that are generated when using the nats command-line tool for creating buckets, storing objects, retrieving objects, etc.

I'm going to start a checklist of functionality I want to cover before we merge/release this API

  • create a bucket
  • validate bucket names before creating
  • put a file in a bucket
  • overwrite a file in a bucket (cleanup prior chunks)
  • put a file in a bucket with TTL
  • get a file from a bucket (provide a callback function to accept each chunk)
  • delete a file from a bucket
  • list the files in a bucket

@mmmries mmmries mentioned this pull request Jul 3, 2023
Copy link
Collaborator

@brandynbennett brandynbennett left a comment

Choose a reason for hiding this comment

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

Looking good so far!

@@ -18,7 +18,7 @@ jobs:
pair:
- otp: "24.3.4"
elixir: "1.12"
nats: "2.2.0"
nats: "2.3.0"
Copy link
Owner Author

Choose a reason for hiding this comment

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

NATS 2.2.0 doesn't support a PURGE with filter: "my subject" which is needed to support deleting objects from a bucket. So I had to bump this to 2.3.0

@mmmries mmmries marked this pull request as ready for review July 5, 2023 11:16
@mmmries
Copy link
Owner Author

mmmries commented Jul 5, 2023

@brandynbennett this is ready for a review now. A couple of things I'm wondering about:

  • I tried to mostly follow the naming convention that I saw in the go client for things like list and info, but wavered for things like create_bucket and put_object. I wonder if we should move bucket operations to a separate module?
    • Bucket.create, Bucket.delete, Bucket.list
    • Object.put, Object.delete, Object.info
  • I should probably add some more type specs and docs for these functions, it would depend on the answer to my last question so didn't want to invest the time until we feel good about the API
  • Release schedule: Should I go ahead and make a release candidate with these changes? That way we could get the code running for an internal tool project as Spiff and have some real world validation?

@mmmries
Copy link
Owner Author

mmmries commented Jul 5, 2023

Since it's still a few hours before the US team is online, I decided to go ahead and release 0.0.8-alpha1 so I could test these changes

# create a random 2MB binary file
# re-use it on subsequent test runs if it already exists
defp generate_big_file do
filepath = Path.join("tmp", "big_file.bin")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we us the ExUnit tmp_dir here?

@@ -24,3 +24,6 @@ jetstream-*.tar

# ASDF tool versions
.tool-versions

# tmp directory to test files
tmp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we us the ExUnit tmp_dir here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍 fixed

:ok

"""
@spec purge(conn :: Gnat.t(), stream_name :: binary()) :: :ok | {:error, any()}
Copy link
Collaborator

Choose a reason for hiding this comment

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

spec is missing method

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍 fixed

@stream_prefix "OBJ_"
@subject_prefix "$O."

def create_bucket(conn, bucket_name, params \\ []) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could see the case for making an Object.Bucket module to separate responsibilities. I think from an interface perspective I would still want to delegate so calling Object.create_bucket is the public interface.

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍 I can get behind that. I'll keep create_bucket and delete_bucket as-is. And Then change the rest of the functions to skip the _object since that focus of this API is to work with the files in a bucket.

@mmmries
Copy link
Owner Author

mmmries commented Jul 6, 2023

I temporarily installed dialyxir into this project locally to validate that my new typespecs are all valid. When I did that, I found some errors from existing parts of this library. So I think we should probably start up a separate PR to add dialyxir to this project and run it in CI to keep ourselves honest with all of our typespecs.

Copy link
Collaborator

@brandynbennett brandynbennett left a comment

Choose a reason for hiding this comment

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

Good idea about Dialyxir pr

@mmmries mmmries merged commit 30ddfb5 into master Jul 6, 2023
2 checks passed
@mmmries mmmries deleted the object-store branch July 6, 2023 17:09
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.

2 participants