[BEAM-5910] Add lastModified field to MatchResult.Metadata #6914

jklukas · 2018-11-01T18:53:44Z

In the Java SDK, the Filesystems.match facilities are aimed primarily at listing file names and collect very limited additional metadata from the filesystem (sizeBytes and isReadSeekEfficient). This PR adds a new lastModified field to that list.

This could be a basis for a future improvement to FileIO.match(...).continuously(...) where we could let the user opt to poll not just for new file names, but also for existing file names if their content has been updated.

In the near term, the addition of lastModified to Metadata will allow users to implement their own polling logic on top of Filesystems.match to detect and download new files from any of the supported filesystems.

Follow this checklist to help us incorporate your contribution quickly and easily:

Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

jklukas · 2018-11-01T18:54:38Z

I'm expecting some tests to fail initially. Seeing what fails will let me know what tests need to be updated.

jklukas · 2018-11-02T17:13:54Z

retest this please

jklukas · 2018-11-02T19:38:27Z

retest this please

iemejia · 2018-11-05T17:01:05Z

Just for comment, the PR looks great, but waiting for the discussion on @dev to see what is the recommended path for the MetadataCoder update.

jklukas · 2018-11-05T18:00:34Z

Thanks for reviewing, @iemejia. I'm very interested to see what comes out of the discussion on evolving coders.

iemejia · 2018-11-06T09:07:02Z

Yes hopefully it won't take long, but probably will require a rebase (assuming that we will version the coders and save this somewhere).

jklukas · 2018-12-27T14:06:03Z

@iemejia - The mailing list discussion looks to have reached a conclusion that there's no short-term solution for coder versioning, so we need to be conservative about compatibility.

I've added a new commit here that returns MetadataCoder to compatibility with the existing format, providing a default value for lastModifiedMillis. We introduce a MetadataCoderV2 that encodes lastModifiedMillis, but is strictly opt-in; it's not used directly by the SDK at all.

Can you take another look and see if this looks good to merge?

iemejia · 2019-01-08T13:12:09Z

Sure @jklukas sorry for the delay will take a look ASAP (Just back from christmass holidays).

iemejia · 2019-01-09T17:05:26Z

Oups just realized I reviewed the wrong one hehe never mind, will start to check this one now. Sorry.

jklukas · 2019-01-09T17:29:26Z

Oups just realized I reviewed the wrong one hehe never mind, will start to check this one now. Sorry.

😆

I really appreciate the reviews, @iemejia, and I'm glad you were able to take time off over the holidays.

iemejia · 2019-01-11T16:14:03Z

Sorry for the delay @jklukas was a bit busy with other stuff + finishing other reviews, your PR is next in the line.

iemejia

Found some minor issues. Please fix and we are almost done.

Can you please add some tests (at least one) to ensure the correct behavior of coding/decoding a Metadata object with MetadataCoderV2. See CoderProperties.structuralValueDecodeEncodeEqual(CODER, value); for reference.

iemejia · 2019-01-17T13:22:46Z

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

        .setIsReadSeekEfficient(isReadSeekEfficient)
        .setSizeBytes(sizeBytes)
-        .build();
+        .setLastModifiedMillis(UNKNOWN_LAST_MODIFIED_MILLIS);


I think this line breaks backwards compatibility (or is at least inconsistent with the encoder), we should remove this and do it only in the V2 version.

The idea here is that lastModifiedMillis must have some default value. I've chosen here to use -1 so that lastModifiedMillis doesn't have to be nullable, though we can discuss if null makes more sense here.

AutoValue requires that values be set for all members. To define a default value, the AutoValue docs suggest a pattern of calling setters before returning the builder. That's what's going on here.

I don't see that there's a backwards incompatibility here. MetadataCoder still only writes and reads the three existing values (resource id, int, and long). When encoding, it throws away lastModifiedMills and when decoding, it provides the default -1 value.

Do you have thoughts on whether null would be preferred as the default vs. -1?

Of note, Java's File.lastModified documents that 0 is used if the file does not exist or an I/O error occurs. We could follow that and use 0.

What if we let zero as a default. In that case we won't need that and we will still be ok with AutoValue. If we setup any default I think it is better to do it at the core object level Metadata.create() better than at other places. Also in the current case if you put this only in decode then encode won't put the right value. Better to left this class as it was to avoid the risk of breaking stuff..

What if we let zero as a default. In that case we won't need that and we will still be ok with AutoValue

To be clear, 0 is only used as a default for File.lastModified as used in LocalFileSystem. Other filesystems do not have this same concept of 0 as default. So any way we go, we will have to explicitly set some default value for the field.

I agree with you that it's better to set the default within Metadata rather than only in the coder, so I will make that change. And will also change the default to be 0 to be consistent with File.lastModified behavior.

Also in the current case if you put this only in decode then encode won't put the right value

Exactly. encode does not encode the lastModifiedMillis value; it's thrown away. And decode provides a default. So encode and decode remain matched in the number and types of encoded values, and these are compatible with previous beam versions.

iemejia · 2019-01-17T14:03:06Z

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

+ * MetadataCoderV2} for retaining timestamp information.
+ */
 public class MetadataCoder extends AtomicCoder<Metadata> {
+  public static final long UNKNOWN_LAST_MODIFIED_MILLIS = -1L;


Remove see comment above

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

jklukas · 2019-01-17T16:42:06Z

Fixed the typos, converted both MetadataCoder and MetadataCoderV2 to singletons, and added tests.

I responded to the comment on UNKNOWN_LAST_MODIFIED_MILLIS. The tests exercise that the new code and validate that behavior is how I intended (MetadataCoder discards lastModifiedMillis when encoding and provides a default value on decoding). I believe that proves backwards compatibility, but do let me know if there's some additional nuance I'm missing there.

jklukas · 2019-01-17T17:36:49Z

Pushed a new commit that uses 0 for the default, moves the default into Metadata, and documents the behavior.

iemejia

LGTM. Fantastic work @jklukas! Thanks for fixing this omission!

…Result.Metadata

iemejia · 2019-01-18T09:25:32Z

Merged manually to squash the commits.

jklukas force-pushed the filesystem-meta-last-modified branch 3 times, most recently from 1c58a88 to 5610fbe Compare November 2, 2018 12:34

iemejia self-requested a review November 5, 2018 14:33

jklukas force-pushed the filesystem-meta-last-modified branch 3 times, most recently from 60702bd to 6d1c746 Compare December 26, 2018 22:13

jklukas force-pushed the filesystem-meta-last-modified branch from 6d1c746 to 8e9177a Compare January 16, 2019 17:30

iemejia requested changes Jan 17, 2019

View reviewed changes

jklukas force-pushed the filesystem-meta-last-modified branch from 0c5cb55 to f4118ef Compare January 17, 2019 16:43

jklukas force-pushed the filesystem-meta-last-modified branch from 919b0b7 to 4ae354b Compare January 17, 2019 17:38

jklukas added 6 commits January 17, 2019 14:22

[BEAM-5910] Add lastModified field to MatchResult.Metadata

db2334d

Add MetadataCoderV2 for encoding lastModifiedMillis

9fdb9a5

Mark new Metadata field and coder @experimental

caa4f64

Document an example of explicitly setting the Metadata coder

b7df051

Have both MetadataCoder variants return singletons

4c7a508

Add tests for MetadataCoder and MetadataCoderV2

3b4bbeb

Set lastModifiedMillis default in Metadata rather than MetadataCoder

6bc7794

jklukas force-pushed the filesystem-meta-last-modified branch from 4ae354b to 6bc7794 Compare January 17, 2019 19:22

iemejia approved these changes Jan 18, 2019

View reviewed changes

iemejia added a commit that referenced this pull request Jan 18, 2019

Merge pull request #6914: [BEAM-5910] Add lastModified field to Match…

137e86a

…Result.Metadata

iemejia closed this Jan 18, 2019

jklukas mentioned this pull request Jun 26, 2019

[BEAM-5191] Support for BigQuery clustering #8945

Merged

3 tasks

pabloem mentioned this pull request Sep 23, 2021

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

Merged

[BEAM-5910] Add lastModified field to MatchResult.Metadata #6914

[BEAM-5910] Add lastModified field to MatchResult.Metadata #6914

Uh oh!

Conversation

jklukas commented Nov 1, 2018

Post-Commit Tests Status (on master branch)

Uh oh!

jklukas commented Nov 1, 2018

Uh oh!

jklukas commented Nov 2, 2018

Uh oh!

jklukas commented Nov 2, 2018

Uh oh!

iemejia commented Nov 5, 2018

Uh oh!

jklukas commented Nov 5, 2018

Uh oh!

iemejia commented Nov 6, 2018

Uh oh!

jklukas commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iemejia commented Jan 8, 2019

Uh oh!

iemejia commented Jan 9, 2019

Uh oh!

jklukas commented Jan 9, 2019

Uh oh!

iemejia commented Jan 11, 2019

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

iemejia Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

jklukas Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

jklukas Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

iemejia Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

jklukas Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

iemejia Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jklukas commented Jan 17, 2019

Uh oh!

jklukas commented Jan 17, 2019

Uh oh!

iemejia left a comment

Choose a reason for hiding this comment

Uh oh!

iemejia commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jklukas commented Dec 27, 2018 •

edited

Loading

iemejia Jan 17, 2019 •

edited

Loading