Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit warning when emit_empty_windows is used unguarded. #851

Merged
merged 5 commits into from
Mar 22, 2021

Conversation

mfelsche
Copy link
Member

And also rework the warnings api for the parser a bit.

This was an oversight from #828

Pull request

Description

We now create a warning like this when only emit_empty_windows is specified without guards like max_groups or eviction_period:

Warning: 
    1 | define tumbling window my_window
    2 | with
    3 |   interval = core::datetime::with_seconds(2),
    4 |   emit_empty_windows = true
    5 | end;
      | ^^^ Using `emit_empty_windows` without guard is potentially dangerous. Consider limiting the amount of groups maintained internally by using `max_groups` and/or `eviction_period`

Related

Checklist

  • The RFC, if required, has been submitted and approved
  • Any user-facing impact of the changes is reflected in docs.tremor.rs
  • The code is tested
  • Use of unsafe code is reasoned about in a comment
  • Update CHANGELOG.md appropriately, recording any changes, bug fixes, or other observable changes in behavior
  • The performance impact of the change is measured (see below)

Performance

@coveralls
Copy link
Collaborator

coveralls commented Mar 18, 2021

Coverage Status

Coverage increased (+0.04%) to 81.702% when pulling 89286b6 on window-empty-warning into 18d4e3a on main.

@mfelsche mfelsche force-pushed the window-empty-warning branch 2 times, most recently from cb83804 to 6112607 Compare March 19, 2021 15:33
darach
darach previously approved these changes Mar 22, 2021
Copy link
Member

@darach darach left a comment

Choose a reason for hiding this comment

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

LGTM

Licenser
Licenser previously approved these changes Mar 22, 2021
Copy link
Member

@Licenser Licenser left a comment

Choose a reason for hiding this comment

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

👍 Nice! I love the const'ed strings!

Matthias Wahl added 5 commits March 22, 2021 14:39
And also rework the warnings api for the parser a bit.

Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
Signed-off-by: Matthias Wahl <mwahl@wayfair.com>
@mfelsche mfelsche dismissed stale reviews from Licenser and darach via 89286b6 March 22, 2021 13:39
@mfelsche mfelsche force-pushed the window-empty-warning branch from 6112607 to 89286b6 Compare March 22, 2021 13:39
@mfelsche mfelsche merged commit 3f779b8 into main Mar 22, 2021
@mfelsche mfelsche deleted the window-empty-warning branch March 22, 2021 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants