[BEAM-13640][BEAM-12883] Add MetadataDynamicCoder to support encode-decode for new fields in Metatdata #15699

brachi-wernick · 2021-10-10T08:45:39Z

This PR is a continuous work for #15510.

Currently there are 2 Coders for Metadata: default one: org.apache.beam.sdk.io.fs.MetadataCoder and enhanced one org.apache.beam.sdk.io.fs.MetadataCoderV2, the last can also decode-encode lastModifiedMillis and it is done in a new coder in order to support backward compatibility.

This will be hard to maintain, we will need to create a new coder for any new field that will be added to Metadata.

So, as suggested in this comment: #15510 (comment), I came up with new generic coder : MetadataDynamicCoder.

MetadataDynamicCoder can decode/encode any new fields added to Metadata by sending getter, setter and coder.

For example creating coder for lastModifiedMillis:

 new MetadataDynamicCoder()
        .withCoderForField(
            VarLongCoder.of(),
            Metadata::lastModifiedMillis,
            Metadata.Builder::setLastModifiedMillis);

I chose to get explicit getter/setter to avoid reflection which has bad impact on performance.

… add-coder-readable-file-v2 # Conflicts: # sdks/java/core/src/main/java/org/apache/beam/sdk/io/fs/ReadableFileCoderV2.java

…Coder

…or any new field that added to Metadata

brachi-wernick · 2021-10-10T09:46:30Z

@pabloem @robertwb new PR submitted #15699 to address MetadataCoder to be more extendable for new fields. can your review?

aaltay · 2021-10-22T01:23:08Z

The other PR seems to be merged. Could we close this?

brachipa · 2021-10-22T04:07:52Z

Please don't, the other PR fixed the ReadableFileCoder, and this one fixes the Mrtadatacoder itself.
To address this suggestion/comment #15510 (comment)

kennknowles · 2021-10-26T19:50:48Z

Adding @pabloem since he reviewed the other PR associated with this Jira, and CC @robertwb

pabloem · 2021-11-09T19:06:22Z

This is pretty cool. Thanks @brachi-wernick !

I think it makes sense to create a JIRA issue with target version 3.0.0 to remove the V1 and V2 coders, and rely directly on the latest versions of these coders. WDYT?

I will ask someone who knows coders better to verify that this coder implementation is backwards-compatible for the addition of new fields.

aaltay · 2021-11-30T17:21:09Z

Folks, what are the next steps on this PR?

brachi-wernick · 2021-11-30T21:34:18Z

@aaltay I would say to merge it as additional dynamic coder(I mean addition to V1 and V2), we can open another JIRA ticket and PR to delete old coders as @pabloem said. WDYT?
This is not critical for now to merge it, but we will need it in the day someone will add more fields to Metadata class.

aaltay · 2021-12-01T18:11:11Z

@aaltay I would say to merge it as additional dynamic coder(I mean addition to V1 and V2), we can open another JIRA ticket and PR to delete old coders as @pabloem said. WDYT?

That sounds reasonable to me. We can merge it once @pabloem completes his review.

aaltay · 2021-12-11T04:16:57Z

@pabloem - a friendly ping for a review.

aaltay · 2022-01-21T00:44:04Z

@pabloem - Could you please take a look?

aaltay · 2022-01-27T02:01:13Z

@pabloem - friendly ping. Could you review or find another reviewer?

aaltay · 2022-02-05T02:25:16Z

@pabloem - friendly ping? Perhaps @kerrydc could help with finding another reviewer?

aaltay · 2022-02-18T04:21:11Z

@chamikaramj @kerrydc - Could you please help to find a reviewer?

github-actions · 2022-04-19T12:55:32Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

aaltay · 2022-04-20T16:53:02Z

@chamikaramj / @johnjcasey - folks could you please review or find a new reviewer?

johnjcasey

Overall this looks good, though I think we will need to consider how we want to go about configuring this in a maintainable way going forward. Adding some fields, and ignoring others, could become complex if we don't have a good pattern.

johnjcasey · 2022-04-20T17:01:49Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/fs/MetadataDynamicCoder.java

+
+  private static final MetadataCoder V1_CODER = MetadataCoder.of();
+
+  private List<Coder<?>> coders = new ArrayList<>();


Do we want to use parallel lists? I would typically create a new pojo of {coder, getter, setter} and have a list of that instead, to avoid off by one errors if we ever modify this class, or need to add a new aspect to the dynamic coder that would warrant a new list

Extracted into class in 420d4aa

Regrading parallel list, do you mean stream.parallel? and decode/encode in parallel? I think it won't work since we iterate the stream sequentially.

Ah no, parallel list for me means when we have 2 or more lists, where listA.get(2) is logically associated with listB.get(2)

johnjcasey · 2022-04-20T17:08:33Z

sdks/java/core/src/test/java/org/apache/beam/sdk/io/fs/MetadataDynamicCoderTest.java

+  @Rule public transient TemporaryFolder tmpFolder = new TemporaryFolder();
+
+  @Test
+  public void testEncodeDecodeWithDefaultLastModifiedMills() throws Exception {


Would it be possible to add a test that uses two dynamic coders, so we can verify that adding multiple coders stack as expected?

Kind problematic, since all fields now in Metadata are handled. except the one that I used in the test.

It can work if I won't use the basic MetadataCoder in this dynamic coder, and work only with the fields coders I get in the list, But I think it will be mess to developers to assign all these basic fields for the dynamic coder, it is easy now, that they need to send only new/special fields and not all the basic fields.

(coder now first does : MatchResult.Metadata.Builder builder = V1_CODER.decodeBuilder(inStream);
which covers most of the fields in Metadata. and new fields will be covered with this coder list of {coder,getter,setter})

Sounds good

…lass (coder, getter, setter)

…add-metadata-dynamic-coder

johnjcasey · 2022-04-26T12:51:52Z

LGTM

asf-ci · 2022-04-26T20:38:02Z

Can one of the admins verify this patch?

asf-ci · 2022-04-26T20:38:02Z

Can one of the admins verify this patch?

asf-ci · 2022-04-26T20:38:02Z

Can one of the admins verify this patch?

asf-ci · 2022-04-26T20:38:02Z

Can one of the admins verify this patch?

pabloem · 2022-04-26T20:38:10Z

reopening PR to triggert tests to re-run

aaltay · 2022-05-05T15:04:33Z

What is the next step on this PR?

aaltay · 2022-05-12T21:33:26Z

Could this be merged? Should it be closed?

johnjcasey · 2022-05-13T13:48:28Z

Run Java Precommit

johnjcasey · 2022-05-13T13:52:58Z

Looks like tests are failing. @brachi-wernick could you investigate and resolve the breaks?

github-actions · 2023-06-20T12:43:26Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

github-actions · 2023-06-27T12:45:24Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

brachipa and others added 10 commits September 14, 2021 23:12

add coder readable files

d2e5106

add coder readable files

058400d

Merge remote-tracking branch 'origin/add-coder-readable-file-v2' into…

25b02e0

… add-coder-readable-file-v2 # Conflicts: # sdks/java/core/src/main/java/org/apache/beam/sdk/io/fs/ReadableFileCoderV2.java

:sdks:java:core:spotlessApply

4dec390

[BEAM-12883] add ability tp set custom MetadataCoder for ReadableFile…

a224a66

…Coder

[BEAM-12883] add ability tp set custom MetadataCoder for ReadableFile…

1e3e452

…Coder

[BEAM-12883] add ability t0 set custom MetadataCoder using Structured…

6cdddfe

…Coder

[BEAM-12883] remove asterisk-based import

6c0906a

[BEAM-12883] add ability for MetadataCoder to support encode-decode f…

d9f6f00

…or any new field that added to Metadata

Merge branch 'apache:master' into add-metadata-dynamic-coder

6f20c30

brachipa mentioned this pull request Oct 13, 2021

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

Merged

kennknowles requested a review from pabloem October 26, 2021 19:50

brachi-wernick changed the title ~~[BEAM-12883] Add MetadataDynamicCoder to support encode-decode for new fields in Metatdata~~ [BEAM-13640][BEAM-12883] Add MetadataDynamicCoder to support encode-decode for new fields in Metatdata Jan 12, 2022

github-actions bot added the stale label Apr 19, 2022

johnjcasey requested changes Apr 20, 2022

View reviewed changes

github-actions bot removed the stale label Apr 21, 2022

[BEAM-13640][BEAM-12883] extract field coder needed stuff to a diff c…

420d4aa

…lass (coder, getter, setter)

github-actions bot added the java label Apr 26, 2022

brachipa added 2 commits April 26, 2022 11:28

Merge branch 'master' of https://github.com/brachi-wernick/beam into …

9082b31

…add-metadata-dynamic-coder

[BEAM-13640][BEAM-12883] fix

f26c9e8

pabloem closed this Apr 26, 2022

pabloem reopened this Apr 26, 2022

[BEAM-13640][BEAM-12883] fix error initialization.fields.uninitialized

35d806b

github-actions bot added the stale label Jun 20, 2023

github-actions bot closed this Jun 27, 2023


		private static final MetadataCoder V1_CODER = MetadataCoder.of();

		private List<Coder<?>> coders = new ArrayList<>();

[BEAM-13640][BEAM-12883] Add MetadataDynamicCoder to support encode-decode for new fields in Metatdata #15699

[BEAM-13640][BEAM-12883] Add MetadataDynamicCoder to support encode-decode for new fields in Metatdata #15699

Uh oh!

Conversation

brachi-wernick commented Oct 10, 2021

Uh oh!

brachi-wernick commented Oct 10, 2021

Uh oh!

aaltay commented Oct 22, 2021

Uh oh!

brachipa commented Oct 22, 2021

Uh oh!

kennknowles commented Oct 26, 2021

Uh oh!

pabloem commented Nov 9, 2021

Uh oh!

aaltay commented Nov 30, 2021

Uh oh!

brachi-wernick commented Nov 30, 2021

Uh oh!

aaltay commented Dec 1, 2021

Uh oh!

aaltay commented Dec 11, 2021

Uh oh!

aaltay commented Jan 21, 2022

Uh oh!

aaltay commented Jan 27, 2022

Uh oh!

aaltay commented Feb 5, 2022

Uh oh!

aaltay commented Feb 18, 2022

Uh oh!

github-actions bot commented Apr 19, 2022

Uh oh!

aaltay commented Apr 20, 2022

Uh oh!

johnjcasey left a comment

Choose a reason for hiding this comment

Uh oh!

johnjcasey Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

brachi-wernick Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

johnjcasey Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

johnjcasey Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

brachi-wernick Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

johnjcasey Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

johnjcasey commented Apr 26, 2022

Uh oh!

asf-ci commented Apr 26, 2022

Uh oh!

asf-ci commented Apr 26, 2022

Uh oh!

asf-ci commented Apr 26, 2022

Uh oh!

asf-ci commented Apr 26, 2022

Uh oh!

pabloem commented Apr 26, 2022

Uh oh!

aaltay commented May 5, 2022

Uh oh!

aaltay commented May 12, 2022

Uh oh!

johnjcasey commented May 13, 2022

Uh oh!

johnjcasey commented May 13, 2022

Uh oh!

github-actions bot commented Jun 20, 2023

Uh oh!

github-actions bot commented Jun 27, 2023