It might be good to be more specific in this message to specify what's being migrated and why.

And I'd suggest putting migration log messages at a different log level than "debug", so people can more readily see what's going on.

The migration logs are at info level. Only the timing stats are on debug level.

Timing stats could be useful to have at the info level if we're trying to track down performance issues with migration. The log line should only ever be displayed once, so I don't think it would be overly verbose to have it at the info level.

Why move this up here? It was originally done inside the transaction after data files has been written to decrease the amount of time this lock needs to be held.

So you would change getWithoutLock to get. And take the lock again before Commit?

I would just keep this whole block where it was. The case where the image already exists does mean that we have unnecessarily written out the metadata files but the transaction will clean that up. I would argue it is better to be optimistic here by writing out the files then potentially throwing them away at the end to reduce the time the layer lock is held. Acquiring the lock twice to avoid this just does not seem necessary.

This function is actually never called in parallel so these locks don't block in migration. Still, it should be correct of course. Copying/writing files when I already know that I won't be using them does not sound like a good idea, especially when this PR is trying to optimize things. I'm fine for the second lock for clarity.

If when this is run there will not be contention for the adding layers then I am fine with leaving it like this. Two locks mean two existence checks and that is not ideal.

i would prefer if we put the variable name here too

should we close this data file? I don't see a call to Close anywhere

Fixed

Could this just be a utility function rather than adding a bool arg here?

so:

w, err := fm.TarSplitWriter()
w = WithCompression(w)

Slight problem with this is that the compressed version is the default that should be always used. Uncompressed (bool=truefalse) is the hack needed for migration.

Right, so the default would be to use only fm.TarSplitWriter() and for the migration use the helper fn.

Below we'd move the close wrapper to the helper, since that's only needed because of the compression, and return f directly (as an io.WriteCloser).

Functional args!

type TarSplitOption func(fm *fileMetadataTransaction)

func (fm *fileMetadataTransaction) TarSplitWriter(options ...TarSplitOption) (io.WriteCloser, error) {
    for _, opt := range options {
        opt(fm)
    }
    ...
}

func CompressInput(fm *fileMetadataTransaction) {
    fm.compressInput = true
}

Sorry, I mixed it up. bool=false is the exception for the migration.

And the issue with bool arguments 😈

@cpuguy83 I can rewrite it the way @aaronlehmann suggested if you find that cleaner.

ping @cpuguy83 ^^ 😇

I'm ok with it as is. No need to make such a change for a single option.