Skip to content

fix zero period in PeriodGranularity causing infinite loop then OOM#3644

Merged
nishantmonu51 merged 1 commit intoapache:masterfrom
kaijianding:period
Nov 2, 2016
Merged

fix zero period in PeriodGranularity causing infinite loop then OOM#3644
nishantmonu51 merged 1 commit intoapache:masterfrom
kaijianding:period

Conversation

@kaijianding
Copy link
Copy Markdown
Contributor

when construct PeriodGranularity with ZERO period like P0D, P0M, P0S... will cause infinite loop when calling gran.iterable(start, end), because the PeriodGranularity.next(long t) never move forward.
Finally, this issue will cause "out of heap space" error when makeCursors(...) and bring down the historical/realtime.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

cool, thanks for the fix. Does DurationGranularity need something similar?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might as well toss this.period = Preconditions.checkNotNull(period, "period"); while we're here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please add a test for this in QueryGranularityTest, ideally one with deserialization.

@gianm gianm added the Bug label Nov 2, 2016
@gianm gianm added this to the 0.9.3 milestone Nov 2, 2016
@kaijianding
Copy link
Copy Markdown
Contributor Author

DurationGranularity doesn't need this check, the construct will throw zero divide error if millis=0

  public DurationGranularity(long millis, long origin)
  {
    this.length = millis;
    this.origin = origin % length;
  }

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Nov 2, 2016

ha, cool :)

It'd be nice to have a check in DurationGranularity anyway, just to have a nicer error message.

@kaijianding
Copy link
Copy Markdown
Contributor Author

checkNotNull and test added, @gianm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggest Assert.fail() here just to make sure the exception is actually thrown. Or, use the @Rule ExpectedException (example in FileTaskLogsTest)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Modified.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Nov 2, 2016

thanks @kaijianding!

@nishantmonu51
Copy link
Copy Markdown
Member

👍

@nishantmonu51 nishantmonu51 merged commit 2961406 into apache:master Nov 2, 2016
fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016
@kaijianding kaijianding deleted the period branch December 15, 2016 15:24
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants