BT-724 Fix BlobPathBuilder failing on retrieving existing filesystem

[kraefrei]

No description provided.

[mspector]

Looks good to me, but the failing test is a bit weird -- gut check says it's not related to your changes, but it doesn't seem like the kind of thing that should just flake out randomly either...

[jgainerdewar]

Hmm, this is a good demonstration but I don't know that we actually want to check it in. This is a good topic to discuss in Michael's meeting next week, how should we be handling automated tests for this stuff?

[kraefrei]

Agreed here, I've been leaving this for the time being because I have been using it to verify I haven't broken these changes, but I can remove this before merge

[jgainerdewar]

Generally in Scala we don't want unprotected .get calls resulting in exceptions, even when we expect and properly handle those exceptions. This is partially style and partially performance - my understanding is that throwing exceptions up and down deep stacks is very expensive, so it's good to get out of the habit.

I think there's a different way to structure this whole block so it returns the InvalidFilePath objects directly rather than throwing and creating them in recovery, we can maybe workshop it tomorrow.

[kraefrei]

Thanks for the feedback here, I figured this wasn't a great way to go, but more generally I wasn't able to find great advice on handling nested try situations overall. I realized now looking at it again, I think the nested try is totally unnecessary 😅 Sounds good!

[jgainerdewar]

It would be good to pass a URI into this function rather than having it access the value on the class and create it multiple times. Total nitpicking, though, no need to hold merging up for that, just something to think about next time we're in here.

[breilly2]

Is this to account for things that are "provider-dependent" as described in the FileSystems JavaDoc? I'm curious exactly what behavior you saw, such as getFileSystem returning a reference to a closed file system or throwing FileSystemNotFoundException. I think it's worth having a link to any documentation that describes any provider-dependent behavior or documenting our observations ourselves.

I'm also still contemplating that potential gap between getFileSystem and newFileSystem. I'd love to avoid adding our own synchronization around this, but I guess that really depends on how the blob filesystem behaves and what failure modes look like and how we want to handle them.

[breilly2]

👍 Still some discovery to do around how the blob filesystem behaves in the context of concurrent access, but that's covered by other tickets.