Skip to content

Conversation

@rohitagarwal003
Copy link
Contributor

Also, fix outdated documentation.

@rohitagarwal003
Copy link
Contributor Author

cc - @kl0u, @aljoscha for review

@rohitagarwal003 rohitagarwal003 force-pushed the FLINK-5247-allowed-lateness branch from 10d5238 to ea83df2 Compare December 21, 2016 04:23
@rohitagarwal003 rohitagarwal003 changed the title [FLINK-5247] Fix check to make sure that we throw error when allowed lateness is set for non event-time windows. [FLINK-5247] Fix incorrect check in allowedLateness() method. Make it a no-op for non-event time windows. Dec 21, 2016
@rohitagarwal003
Copy link
Contributor Author

Ping @aljoscha

Copy link
Contributor

@kl0u kl0u left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mindprince . Thanks for the work. The changes look good. My only comment is that in case of a processing time assigner, we should also log a warning that the allowed lateness is set to 0 (stating also the reason), instead of doing it silently. In general, I think that silently overriding user-specified parameters is not the best practice.

@rohitagarwal003
Copy link
Contributor Author

Thanks for the review @kl0u

I didn't do it because there was no logger defined in the file.

@kl0u
Copy link
Contributor

kl0u commented Jan 6, 2017

Hi @mindprince . What do you mean "in the file"? The allowed lateness can always be specified in the program by the developer, right? So logging a message is always a valid approach.

@xhumanoid
Copy link
Contributor

I think @mindprince mean: at now logger not defined in class file

this classes without
private static final Logger LOG

@StephanEwen
Copy link
Contributor

Thank you for fixing this.
I think it is good as it is, will merge this for Flink-1.2 and master...

@rohitagarwal003
Copy link
Contributor Author

Merged in 87af841 and 4697b97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants