stream ACS snapshot dumps to S3

[isegall-da]

Currently dumps all snapshots. Computing which snapshots to actually dump, and which to skip, is tracked as a followup step in the tracking issue.

Fixes #3617

Pull Request Checklist

Cluster Testing

• If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
• If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

• Include any change that might be observable by our partners or affect their deployment in the release notes.
• Specify fixed issues with Fixes #n, and mention issues worked on using #n
• Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

• Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

[isegall-da]

ping @ray-roestenburg-da @rautenrieth-da can I have a review pls?

[isegall-da]

ping @ray-roestenburg-da @rautenrieth-da can I have a review pls?

Ah, sorry, misremembered when I opened this, thought it was much earlier.....

[ray-roestenburg-da]

nice

[ray-roestenburg-da]

Suggested change

	/** *
	/**

[ray-roestenburg-da]

nice!

[ray-roestenburg-da]

style nitpick, you could do SingleAcsSnapshotBulkStorage.getSource and just pass the args to that method, you don't really need to create a new instance of SingleAcsSnapshotBulkStorage and then call it's one method

[isegall-da]

But it has other private methods that access these arguments, so moving them to be arguments of getSource would require passing these around to other private methods as well. Not sure what you gain here...

[ray-roestenburg-da]

yeah I think that's more straightforward, and slightly more functional style, just methods calling methods and if needed passing along arguments. From perspective of the user of the function, the user just calls the function with the args, the user does not need to create an instance just to call one method, so from usage perspective it's simpler, just call the function on object SingleAcsSnapshotBulkStorage

[isegall-da]

Hmmm... still not sure I'm convinced :)
This is hidden in the object's apply, so the user is unaware of the fact that an object is created behind the scenes. The user indeed just calls a method on the object, so that's the UX you propose. Isn't it?

[ray-roestenburg-da]

yes I'm proposing Object.method, no apply, just a method on an object, it's very easy to find. It's not super important 😄

[isegall-da]

ACK, no strong feelings here. The only difference is that a user needs to type also .method after Object when using this, but if that seems more intuitive for you, I'm fine with that.

[ray-roestenburg-da]

You get the source (getSource) which return Source[String, ..], then you fold over it and just emit migration timestamp, this is not complete right? (maybe make a note of that) or am I missing something?

[ray-roestenburg-da]

Maybe I missed the TODOs in getSource. I think it would make sense for the write to s3 to be a flow that emits (migrationid, timestamp) that was written, but you're probably planning this for later?

[isegall-da]

You mean to move the two lines below into the source itself?

[isegall-da]

Guess that makes sense, will do

[ray-roestenburg-da]

Looking at this test, maybe it's nice to separate the code into a source that gives you acs snapshots (from the store) and a flow to write the s3 stuff, and then connect them in the code. (you could also unit test just the flow for instance without having an actual source that gives you real acs snapshots from the db)

[isegall-da]

As things are right now, wrapping the write to s3 as a flow is 2 lines of code, so I don't see great value in adding a class for that. Having said that, there's this TODO where I do plan on doing basically that:

splice/apps/scan/src/main/scala/org/lfdecentralizedtrust/splice/scan/store/bulk/AcsSnapshotBulkStorage.scala

Lines 94 to 97 in b1eb83a

	// TODO(#3429): For now, we accumulate the full object in memory, then write it as a whole.
	// Consider streaming it to S3 instead. Need to make sure that it then handles crashes correctly,
	// i.e. that until we tell S3 that we're done writing, if we stop, then S3 throws away the
	// partially written object.

[ray-roestenburg-da]

you can add a couple of methods on an object (don't need a class) to create the separate sources and flows and then use those somewhere compose them source.via(flow). Not a huge deal.
I'm used to doing something like:

object Bulk {
  def acsFromScan(...): Source[.., ..]
  def acsToS3(...): Flow[..,..,..] 
}

And somewhere you compose the whole stream acsFromScan().via(acsToS3)...
then you can test both separately, and use where you like. just FYI. in a test you can do Source(Vector(acs1,...)).via(acsToS3) .. etc

[isegall-da]

ACK, will do as part of that TODO

[ray-roestenburg-da]

Thanks! definitely going in the right direction, made some comments