[BEAM-12883] Add coder for ReadableFileCoder that supports MetadataCoderV2

[brachi-wernick]

Currently, there is a separated coder for Metadata which cares about Metadata#lastModifiedMillis().
The issue is that the ReadableFileCoder always uses the default Metadata coder.
We need to create a similar coder for ReadableFileCoder: ReadableFileCoderV2 which just use the corresponding MetadataCoderV2.

Without doing so lastModifiedMillis will remain zero.

There is also no way to create custom coder in our side since FileIO.ReadableFile has no public method to initiate new instance of it.

Also check this thread in StackOverflow

[pabloem]

Maybe it makes better sense to ensure the MetadataCoder will actually encode+decode the metadata's lastModifiedMillis field? Why did you choose to not make that change instead? I think that would work best.

cc: @reuvenlax

[brachi-wernick]

Maybe it makes better sense to ensure the MetadataCoder will actually encode+decode the metadata's lastModifiedMillis field? Why did you choose to not make that change instead? I think that would work best.

cc: @reuvenlax

@pabloem why initially this separation was needed between the coders? MetadataCoder and MetadataCoderV2, I just keep this style in the ReadableFileCoder.

[pabloem]

good observation - here's the PR for the first V2 coder (#6914)

@robertwb do you have thoughts on adding a new coder version vs the need to preserve compatibility here?

[robertwb]

I think the only way to preserver backwards compatibility here is to add a new coder and also add an opt-in mechanism for users to select this new coder. It may be worth considering making this coder more extensible so this doesn't happen in the future (e.g. storing the values as a key-value map and ignoring unknown fields, which is less efficient but probably doesn't matter for this coder vs. the chances that we'll want to add a new field in the future.)

[brachi-wernick]

Creating more extendable coder is definitely better, I would suggest the bellow:

1. Add MetadataCoder the ability to get list of fields names needed to be encoded/decode.
2. ReadableFileCoder will get MetadataCoder instance in its constructor parameters. to make the configuration and adjustments only in one place(MetadataCoder)

We can make this also in 2 phases-PRs, (start with number 2 just to solve the current issue and then enhance MetadataCoder for next future fields)

[pabloem]

alright taking a look...

[pabloem]

this asterisk-based import is causing the PreCommit to fail (we try to avoid this sort of import) - can you import only the classes you need?

[pabloem]

LGTM. @brachi-wernick are you able to follow up with change #1?

[brachipa]

are you able to follow up with change #1?

I will work on it and open a diff PR for it and link it to that PR thanks!

[brachipa]

LGTM. @brachi-wernick are you able to follow up with change #1?

In case you missed it, I submitted a PR #15699 for change 1.