[BEAM-10292] DefaultFilenamePolicy.ParamsCoder is not able to decode directory on the local file system by davidak09 · Pull Request #12050 · apache/beam

davidak09 · 2020-06-22T13:46:47Z

DefaultFilenamePolicy.ParamsCoder used StringUtf8Coder for encoding/decoding baseFilename and therefore information whether baseFilename resourceID is file or directory was lost. Using ResourceIdCoder instead fixes this issue.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
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.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Dataflow	Samza	Spark
Go		---	---
Java
Python			---
XLang	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

davidak09 · 2020-06-22T13:48:14Z

R: @dmvk

dmvk

This makes total sense, nice catch 👍

Unfortunatelly backward incompatible changes of coders are blockers for upgrades of streaming pipelines. Beam currently doesn't have any mechanism for coder versioning, so this may be tricky to implement correctly.

In this case, it may be possible to simply append a boolean coder and use it only when more bytes are available for consuption, thus making it backward compatible, eg.:

ResourceId prefix =
  FileBasedSink.convertToFileResourceIfPossible(stringCoder.decode(inStream));
String shardTemplate = stringCoder.decode(inStream);
String suffix = stringCoder.decode(inStream);
boolean isDirectory = false; // or whatever the default is
if (inStream.available() > 0) {
  isDirectory = booleanCoder.decode(inStream);
}

Thanks for the contribution!

@lukecwik any thoughts about this one?

davidak09 · 2020-06-23T06:04:03Z

Thanks for the review @dmvk !

You're right, I forgot to ensure backward compatibility. I tried to implement the requested changes, will you take a look at it please?

dmvk · 2020-06-24T09:11:03Z

Run Java PreCommit

dmvk · 2020-06-24T09:22:32Z

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

Would it be possible to add test case for bw compatible code path?

ok, I added the requested test for bw compatibility

btw you can see the buggy behavior there (lost information whether baseFilename is file/directory)

convertToFileResourceIfPossible attempts to match a file and if that fails attempts to match a directory.

Is the issue that the underlying filesystem erroneously says something is a file when really it is a directory?

Exactly, that's the issue, for example LocalFileSystem matches path /tmp/dirA/ as file..

How about something like this?

public static class ParamsCoder extends AtomicCoder<Params> { private static final ParamsCoder INSTANCE = new ParamsCoder(); private static final Coder<String> STRING_CODER = StringUtf8Coder.of(); public static ParamsCoder of() { return INSTANCE; } @Override public void encode(Params value, OutputStream outStream) throws IOException { if (value == null) { throw new CoderException("cannot encode a null value"); } ResourceId baseFilename = value.getBaseFilename().get(); String baseFilenameString = (baseFilename.isDirectory() && !baseFilename.toString().endsWith("/")) ? baseFilename.toString() + "/" : baseFilename.toString(); STRING_CODER.encode(baseFilenameString, outStream); STRING_CODER.encode(value.getShardTemplate(), outStream); STRING_CODER.encode(value.getSuffix(), outStream); } @Override public Params decode(InputStream inStream) throws IOException { String baseFilenameString = STRING_CODER.decode(inStream); ResourceId prefix = FileSystems.matchNewResource(baseFilenameString, baseFilenameString.endsWith("/")); String shardTemplate = STRING_CODER.decode(inStream); String suffix = STRING_CODER.decode(inStream); return new Params() .withBaseFilename(prefix) .withShardTemplate(shardTemplate) .withSuffix(suffix); } }

But I'm not sure how to handle the suffix (/) to ensure compatibility among (file) systems

I think that there are a couple of options here:

Take the breaking change to the coder because it is a fix, make sure it is documented in the release notes

Try to fix the underlying filesystem to do a better job of file/dir matching

Deprecate this filename policy, create a new one (DefaultFilenamePolicy2) and tell people to use it in new code.

I'm for 1 but would ask for consensus on the mailing list.

Also, any / hacking will make things worse since different file systems use different path separator characters (e.g linux vs windows)

OK, I agree with the first solution - taking the breaking change. I will ask on the mailing list.

BTW I did workaround in my project where I use only files (not directories) as Params base filenames and then everything works fine. That means I no longer needs this fix but it still seems to me like a bug which is worth fixing.

(P.S.: The suffix wouldn't have to be the path separator (/) but e.g. sometotallyrandomstring123 - but it's very ugly...)

https://lists.apache.org/thread.html/r1d844830648b88948d38d0c28d106dd7fe2ed92af4a170e2679f8c2f%40%3Cdev.beam.apache.org%3E

Hi @lukecwik, finally after the discussion in the mailing list I chose the second proposed option and fixed the underlying filesystem. Turns out that only LocalFileSystem is broken, on the other hand e.g. HDFS and S3 look fine and already have this kind of check.

I still think that the proper solution should be to use the ResourceIdCoder instead of StringUtf8Coder but I understand the consequences.

lukecwik

This makes total sense, nice catch 👍

Unfortunatelly backward incompatible changes of coders are blockers for upgrades of streaming pipelines. Beam currently doesn't have any mechanism for coder versioning, so this may be tricky to implement correctly.

In this case, it may be possible to simply append a boolean coder and use it only when more bytes are available for consuption, thus making it backward compatible, eg.:
ResourceId prefix =
  FileBasedSink.convertToFileResourceIfPossible(stringCoder.decode(inStream));
String shardTemplate = stringCoder.decode(inStream);
String suffix = stringCoder.decode(inStream);
boolean isDirectory = false; // or whatever the default is
if (inStream.available() > 0) {
  isDirectory = booleanCoder.decode(inStream);
}
Thanks for the contribution!

@lukecwik any thoughts about this one?

This won't work since you can have multiple encoded Params concatentated together on the input stream so you'll read the first byte of the next Params with this change.

lukecwik · 2020-06-24T17:39:44Z

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

convertToFileResourceIfPossible attempts to match a file and if that fails attempts to match a directory.

Is the issue that the underlying filesystem erroneously says something is a file when really it is a directory?

…gumentException when the first 'singleResourceSpec' parameter ends with filesystem separator and the second provided parameter indicates that it isn't directory. Fixed due to DefaultFilenamePolicy.ParamsCoder was not able to decode directory on the local file system because it calls FileBasedSink.convertToFileResourceIfPossible method which internally calls FileSystem.matchNewResource method. Other file systems (HDFS, S3) already have this kind of check.

lukecwik

Please perform the one fix up and then this is mergeable.

lukecwik · 2020-08-19T17:01:53Z

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

    return constructed.toString();
  }

+  private static DefaultFilenamePolicy.Params encodeDecodeParams(


Use CoderUtils.clone

thanks for the advice 👍

lukecwik · 2020-08-19T17:06:59Z

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


  @Override
  protected LocalResourceId matchNewResource(String singleResourceSpec, boolean isDirectory) {
+    if (singleResourceSpec.endsWith(File.separator) && !isDirectory) {


Do you want to add the file separator if it doesn't exist if isDirectory == true. This will help make it less error prone for users. Similar to GcsFileSystem.

sounds good, I added it

lukecwik

We should update LocalResourceId and drop the isDirectory field

lukecwik · 2020-08-19T17:20:39Z

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


  @Override
  protected LocalResourceId matchNewResource(String singleResourceSpec, boolean isDirectory) {
+    if (singleResourceSpec.endsWith(File.separator) && !isDirectory) {


What if the character is escaped and being used in the name component of the path string?

Is it even possible? For example according to https://stackoverflow.com/questions/9847288/is-it-possible-to-use-in-a-filename it's not. Or do you mean something different?

Your right, it seems as though Linux doesn't allow /, Mac OS X doesn't allow :, Windows doesn't allow \.

- adding file separator if it doesn't exist if isDirectory == true in LocalFileSystem.matchNewResource - dropped isDirectory field in LocalResourceId - using CoderUtils.clone method in DefaultFilenamePolicyTest.testParamsCoder test

davidak09 · 2020-08-20T06:41:21Z

We should update LocalResourceId and drop the isDirectory field

I removed the isDirectory field, nevertheless I left isDirectory parameter in the constructor and the factory method untouched. WDYT?

lukecwik · 2020-08-20T16:59:05Z

We should update LocalResourceId and drop the isDirectory field

I removed the isDirectory field, nevertheless I left isDirectory parameter in the constructor and the factory method untouched. WDYT?

Makes sense since LocalResourceId constructor adds File.separator onto the path.

lukecwik · 2020-08-20T17:01:43Z

@dmvk The backwards compat issues have been addressed. Will merge, feel free to add additional comments if there was something that was not addressed.

probot-autolabeler bot added the java label Jun 22, 2020

dmvk requested changes Jun 22, 2020

View reviewed changes

davidak09 force-pushed the BEAM-10292 branch from a0916e5 to 7384998 Compare June 23, 2020 05:56

dmvk reviewed Jun 24, 2020

View reviewed changes

lukecwik requested changes Jun 24, 2020

View reviewed changes

davidak09 force-pushed the BEAM-10292 branch 2 times, most recently from 3f4aa11 to 7b440f2 Compare August 3, 2020 11:27

davidak09 force-pushed the BEAM-10292 branch from 88dd8b6 to 1cdb8f4 Compare August 11, 2020 12:08

davidak09 changed the title ~~[BEAM-10292] DefaultFilenamePolicy.ParamsCoder uses ResourceIdCoder~~ [BEAM-10292] DefaultFilenamePolicy.ParamsCoder is not able to decode directory on the local file system Aug 11, 2020

davidak09 requested review from dmvk and lukecwik August 11, 2020 13:17

lukecwik approved these changes Aug 19, 2020

View reviewed changes

lukecwik reviewed Aug 19, 2020

View reviewed changes

[BEAM-10292] changes after review

a20b613

- adding file separator if it doesn't exist if isDirectory == true in LocalFileSystem.matchNewResource - dropped isDirectory field in LocalResourceId - using CoderUtils.clone method in DefaultFilenamePolicyTest.testParamsCoder test

lukecwik merged commit d86eccd into apache:master Aug 20, 2020

Conversation

davidak09 commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

davidak09 commented Jun 22, 2020

Uh oh!

dmvk left a comment

Choose a reason for hiding this comment

Uh oh!

davidak09 commented Jun 23, 2020

Uh oh!

dmvk commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Jun 25, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Aug 19, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidak09 commented Aug 20, 2020

Uh oh!

lukecwik commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukecwik commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidak09 commented Jun 22, 2020 •

edited

Loading

lukecwik Jun 25, 2020 •

edited

Loading

lukecwik Aug 19, 2020 •

edited

Loading

lukecwik commented Aug 20, 2020 •

edited

Loading