Add support for custom DateTime patterns for WindowedFilenamePolicy class #159

melbrodrigues · 2020-08-13T13:44:06Z

Allows WindowedFilenamePolicy class to use a custom DateTime pattern for the output directory.

Resolves #155

sabhyankar

Take a look at the review comments. My suggestion would be to look at a builder/fluent pattern so as to not make the constructor look unweildy.

You could look at an example of the pattern here.

sabhyankar · 2020-08-13T14:39:07Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Can this never be null?

I was setting an empty String as the default value for customDateTimePattern. Added a check now to avoid the NPE.

sabhyankar · 2020-08-13T14:39:38Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

You would get an NPI if customDatetimePattern is a null.

sabhyankar · 2020-08-13T14:43:22Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

I think what we need is a way for the user to override what the pattern would be for each of the 5 components (year, month, day, hour and minute).

For example, a user may specify that the replacement pattern for minute should be mx instead of the default mm.

What should the input format for this option be? A comma separated String of the overridden values? Something like mm=mx,HH=hx...

The fluent builder pattern can have methods that allow us to override the patterns for each of these. So something like this:

WindowedFilenamePolicy policy = WindowedFilenamePolicy.of(<existing_input_params>); ... // Override minute pattern from default "mm" to "xx" policy.setMinutePattern("xx"); // Override day pattern from default "DD" to "ZZ" policy.setDayPattern("ZZ");

So does this mean each template that uses the WindowedFilenamePolicy class would have to have a separate input parameter for each Date component? Like:

WindowedFilenamePolicy.newBuilder() .withOutputDirectory(options.getOutputDirectory()) .withOutputFilenamePrefix(options.getOutputFilenamePrefix()) .withShardTemplate(options.getOutputShardTemplate()) .withSuffix(options.getOutputFilenameSuffix()) .withYearPattern(options.getYearPattern()) .withMonthPattern(options.getMonthPattern()) // and so on .build();

Currently, I've changed the class to use the Builder pattern as follows:
(assuming we're taking all the patterns to be overridden in one input parameter --customDateTimePattern from the user)

WindowedFilenamePolicy.newBuilder() .withOutputDirectory(options.getOutputDirectory()) .withOutputFilenamePrefix(options.getOutputFilenamePrefix()) .withShardTemplate(options.getOutputShardTemplate()) .withSuffix(options.getOutputFilenameSuffix()) .withPattern(options.getCustomDateTimePattern()) .build();

What should the format for options.getCustomDateTimePattern() be?

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

sabhyankar · 2020-08-31T14:34:28Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Why do we need a default template given that we didnt have one earlier?

Ah, I got these from the default values set in the Pubsub to Text template. I'll take them out.

sabhyankar · 2020-08-31T14:35:11Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Same here - Why would this be needed now?

sabhyankar · 2020-08-31T14:36:05Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

A newBuilder() should return a Builder and not the actual annotated class.

I'll change this to writeWindowedFiles() since we're not returning a Builder.

sabhyankar · 2020-08-31T14:44:49Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

I prefer we call this (and below) yearPattern() so as to make it less ambiguous.

sabhyankar · 2020-08-31T14:45:28Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Why do we have both a toBuilder() and a newBuilder()?

toBuilder() would be called internally after a withX() method is called.

Changing newBuilder() to writeWindowedFiles()

sabhyankar · 2020-08-31T14:51:07Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Do you think it would be useful to log this at a debug level perhaps? At most we have a log every minute? See how this is done using MoreObjects.firstNotNull

https://github.com/apache/beam/blob/aa345dbeb9f6bdb36f5acf0fb8958777253acd51/sdks/java/io/splunk/src/main/java/org/apache/beam/sdk/io/splunk/SplunkEventWriter.java#L119-L129

Yup, I'll add a few log statements for this

stale · 2020-10-17T20:17:02Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

melbrodrigues · 2020-10-19T06:37:51Z

Hi @sabhyankar , Would be great to get your feedback on this.

sabhyankar · 2020-10-20T23:53:19Z

Hi @melbrodrigues - I will take a look at this.

an2x · 2021-06-21T19:53:26Z

I'm not sure how this change resolves the original issue #155. The problem was that, for example, "mm" in "recommendation" in the directory name was getting replaced with minutes.

Now, with this change, if I want to have the directory as something like gs://bucket/recommendations/<minute> what do I do?
If I set minutePattern to m or mm, it's still going to replace all the letters m in "recommendations".
If I set minutePattern to something like %mm and the directory to gs://bucket/recommendations/%mm, it still won't work properly: the result will be gs://bucket/recommendations/%12 (% won't be replaced).

I suggest implementing a slightly different fix:

Introduce just one new parameter, let's call it directoryDateTimePattern, example value that the user would set: YYYY/MM/DD/HH-mm.
In the directory name, replace ${dateTime} placeholder with the datetime formatted using directoryDateTimePattern. If the placeholder doesn't exist, fallback to the old logic (replace YYYY, MM, DD, etc. separately), for backwards compatibility.
The directory specified by the user would look like gs://bucket/recommendations/${dateTime}.

I think this would be better because this approach is more flexible (what if I don't want minutes in my directory name at all?) and there is no need to set 5 different parameters (and if you do, you may run into many other issues, e.g. what if minutePattern is overridden but hourPattern isn't - is this a misconfiguration?).

melbrodrigues · 2021-06-22T09:26:21Z

I think the intention was to give users the ability to override default patterns and avoid unintentional changes to their file paths.

For the gs://bucket-name/recommendations/ example, the user need only pass one additional parameter --minutePattern (which can be set to say mmmm) to override the default pattern mm.

And if the user wants to include patterns in the directory name they can still do so with specific overrides. For example, an override of --minutePattern=mmmm would result in the path gs://bucket-name/recommendations/mmmm being transformed as gs://bucket-name/recommendations/<minute>.

Note that the user does not need to pass all parameter overrides and the overrides must be a valid pattern as documented in DateTimeFormatter.

an2x · 2021-06-22T23:26:38Z

src/main/java/com/google/cloud/teleport/io/WindowedFilenamePolicy.java

Suggested change

LOG.error("Invalid DateTime pattern: {}", e.getMessage());

throw new RuntimeException(e);

throw new RuntimeException("Invalid DateTime pattern", e);

Usually logging and re-throwing the same exception is considered an anti-pattern. Any strong reason to do it here?

Not really. I saw it at a few other places in the repo back then. I can take out the log statements.

Should I remove the try-catch block as well? The catch block was just to log the error message and throw a new RuntimeException. If I remove the log statement all we're doing is catching an IllegalArgumentException and throwing a RuntimeException.

I'd keep it because IllegalArgumentException doesn't make it clear for the user what pattern is incorrect (e.g. if an empty pattern is supplied it just says "Invalid pattern specification"). Here the RuntimeException at least says that the error was caused by a DateTime pattern.

I'd go even further and include even more details in the error message, e.g. what pattern is incorrect (add a helper method to avoid try-catch around each of the 5 patterns). But it's up to you if you want to make such a change.

I was wondering if it's a good idea to throw a RuntimeException... Since this resolution happens after we read from Pub/Sub, users may lose some data. Maybe we should use the default values if the pattern provided is invalid? WDYT?

I'd still throw the Exception. If a user set a pattern override, this means the default pattern doesn't work for them for some reason. If the override was set incorrectly, simply ignoring this and falling back to the fault pattern may cause unwanted side effects (e.g. output written to a wrong folder, potentially overwriting existing data). IMO it's better to throw an exception and not process anything in this case.

As for the data loss, it shouldn't happen. Dataflow guarantees "exactly once" processing semantics (some info about it). In case of PubSub, messages read from a topic are not going to be ACKed if there is a failure in the middle of the pipeline and PubSub is going to re-deliver them on the next processing attempt.

Perfect. I'll leave the RuntimeException there then

an2x · 2021-06-22T23:42:33Z

src/main/java/com/google/cloud/teleport/templates/PubsubToText.java

I suggest moving all these options (+ the old output* options like outputShardTemplate) to a separate class (e.g. WindowedFilenamePolicyOptions), similar to how StreamingOptions are extended here, to avoid duplication in PubsubToAvro and PubsubToText templates. I'd put it in com.google.cloud.teleport.options package.

Makes sense. Will move these to a new WindowedFilenamePolicyOptions interface

an2x · 2021-06-22T23:49:41Z

src/test/java/com/google/cloud/teleport/io/WindowedFilenamePolicyTest.java

Please add a test to verify that when a custom pattern is set the default pattern is not being replaced.
E.g. if the directory is something like ".../recommendations/mmmm" and the minutePattern is mmmm, then the resulting directory is ".../recommendations/0047" and not ".../reco47endations/0047".

Ah yes, I had added a test for this after our discussion yesterday. Will push it along with the other changes.

an2x · 2021-06-22T23:52:45Z

And if the user wants to include patterns in the directory name they can still do so with specific overrides. For example, an override of --minutePattern=mmmm would result in the path gs://bucket-name/recommendations/mmmm being transformed as gs://bucket-name/recommendations/<minute>.

OK, got it, thanks. I think this is good enough. I left a few other comments, should be the last review round :)

Taking over this review from sabhyankar.

…lass.

zhoufek · 2021-07-15T13:04:43Z

src/main/java/com/google/cloud/teleport/options/WindowedFilenamePolicyOptions.java

@@ -0,0 +1,64 @@
+package com.google.cloud.teleport.options;


This file seems to be the cause of the import/copybara failure preventing merging. You'll need to add a license header similar to the other files. You could copy/paste from another file, though please update the year to 2021.

Thanks, I'll add that.

mvn clean install seems to complain about a missing package-info file for the new options package, so I'll add that as well.

google-cla bot added the cla: yes The PR submitter has a CLA label Aug 13, 2020

melbrodrigues requested a review from sabhyankar August 13, 2020 13:44

sabhyankar reviewed Aug 13, 2020

View reviewed changes

melbrodrigues force-pushed the master branch from a0b9153 to ced606c Compare August 20, 2020 12:27

melbrodrigues requested a review from sabhyankar August 20, 2020 12:30

sabhyankar previously requested changes Aug 31, 2020

View reviewed changes

melbrodrigues force-pushed the master branch from ced606c to 6ea64b7 Compare September 1, 2020 08:15

melbrodrigues requested a review from sabhyankar September 1, 2020 08:24

stale bot added the obsolete The PR hasn't had activity in 45 days label Oct 17, 2020

stale bot closed this Oct 31, 2020

an2x self-requested a review June 21, 2021 16:11

an2x reopened this Jun 21, 2021

stale bot removed the obsolete The PR hasn't had activity in 45 days label Jun 21, 2021

an2x requested changes Jun 22, 2021

View reviewed changes

melbrodrigues force-pushed the master branch from 964a873 to afd7189 Compare June 28, 2021 12:03

melbrodrigues requested a review from an2x June 28, 2021 12:04

an2x approved these changes Jul 9, 2021

View reviewed changes

an2x added the Google LGTM Approval of a pull request to be merged into the repository label Jul 9, 2021

an2x closed this Jul 13, 2021

an2x reopened this Jul 13, 2021

prathapreddy123 added Google LGTM Approval of a pull request to be merged into the repository and removed Google LGTM Approval of a pull request to be merged into the repository labels Jul 13, 2021

melbrodrigues force-pushed the master branch from afd7189 to ebe70ba Compare July 15, 2021 08:02

Add support for custom DateTime patterns for WindowedFilenamePolicy c…

e2f8a20

…lass.

melbrodrigues force-pushed the master branch from ebe70ba to e2f8a20 Compare July 15, 2021 08:21

zhoufek requested changes Jul 15, 2021

View reviewed changes

Add license header and package-info for 'options'

e0aa718

zhoufek approved these changes Jul 15, 2021

View reviewed changes

ryanmcdowell merged commit 297ad21 into GoogleCloudPlatform:master Jul 15, 2021

	LOG.error("Invalid DateTime pattern: {}", e.getMessage());
	throw new RuntimeException(e);
	throw new RuntimeException("Invalid DateTime pattern", e);

Add support for custom DateTime patterns for WindowedFilenamePolicy class #159

Add support for custom DateTime patterns for WindowedFilenamePolicy class #159

Uh oh!

Conversation

melbrodrigues commented Aug 13, 2020

Uh oh!

sabhyankar 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!

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!

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!

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!

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!

stale bot commented Oct 17, 2020

Uh oh!

melbrodrigues commented Oct 19, 2020

Uh oh!

sabhyankar commented Oct 20, 2020

Uh oh!

an2x commented Jun 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melbrodrigues Jun 23, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

melbrodrigues commented Jun 22, 2021 •

edited

Loading

melbrodrigues Jun 23, 2021 •

edited

Loading