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

use latest sketches-core-0.13.1 #7320

Merged
merged 3 commits into from
Apr 3, 2019

Conversation

AlexanderSaydakov
Copy link
Contributor

Must be compatible

@AlexanderSaydakov
Copy link
Contributor Author

Can we make sure this gets into the 0.14.0 release?

@gianm
Copy link
Contributor

gianm commented Apr 2, 2019

Hi @AlexanderSaydakov,

0.14.0 is code-frozen other than regressions / critical bug fixes right now, but if this is committed soon, it will end up in 0.15.0 (which should be code frozen in about 1 month's time). Could you please double check the CI errors? They look legit but might indicate a borked merge, since they seem unrelated to your changes:

[ERROR] /home/travis/build/apache/incubator-druid/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:[7687,40] cannot find symbol
  symbol:   variable RESULT_FORMAT_COMPACTED_LIST
  location: class org.apache.druid.query.scan.ScanQuery

Merging master into your branch should help.

@AlexanderSaydakov
Copy link
Contributor Author

The update to sketches-core-0.13.1 that I committed today fixes a bug in quantiles sketch direct-memory mode that is used in Druid. It was reported a few days ago by Druid users here in Yahoo. So perhaps we should reconsider that "extension" area label on this pull request.

@AlexanderSaydakov
Copy link
Contributor Author

Regarding the build failure, it seems unrelated to my change. The change is just one line in pom.xml to update sketches-core from 0.12.0 to 0.13.1

@jihoonson
Copy link
Contributor

The CI failure is because of #7401. Would you please merge master once #7401 is merged?

@AlexanderSaydakov AlexanderSaydakov changed the title use latest sketches-core-0.13.0 use latest sketches-core-0.13.1 Apr 2, 2019
@AlexanderSaydakov
Copy link
Contributor Author

Would you please merge master once #7401 is merged?

Would it be easier to submit another request with this one-line change based on the fresh master?

@jihoonson
Copy link
Contributor

No. After #7401 is merged, you can simply run git checkout datasketches_0_13_0; git pull master to merge master into your branch. Then, this PR would be automatically updated and trigger Travis CI again if you push your change to your branch.

@AlexanderSaydakov
Copy link
Contributor Author

Could we reconsider the "Extension" category label on this request? sketches-core-0.13.1 has a critical bug fix. We would like to make it a part of a release as soon as possible. Thank you.

@jon-wei jon-wei added the Bug label Apr 3, 2019
@jon-wei
Copy link
Contributor

jon-wei commented Apr 3, 2019

"Extension" just means that the PR affects code that's in an extension. I've added the "Bug" label.

Can you summarize the bug that's fixed?

Copy link
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.

👍 - CI passed, will merge this.

@gianm gianm merged commit 28b4e85 into apache:master Apr 3, 2019
@gianm gianm added this to the 0.15.0 milestone Apr 3, 2019
@AlexanderSaydakov
Copy link
Contributor Author

Can you summarize the bug that's fixed?

Merging quantiles sketch produced wrong results sometimes.
https://github.com/DataSketches/sketches-core/releases/tag/sketches-core-0.13.1

I misunderstood what the "extension" means.
So with the "bug" added would it qualify for inclusion into 0.14.0?

@gianm
Copy link
Contributor

gianm commented Apr 4, 2019

My inclination would be no, since 0.14.0 is in the final stages of approval right now with the IPMC and I don't think it's worth further delay of the release unless something awful is discovered. The considerations I'd use in forming this opinion are,

  • number of users likely to hit the bug (the more users, the more likely we'd want to include it)
  • did this bug exist in the last release or not? (if it's a new bug / regression since the last release, it's more likely to be included; if it existed in the last release too, then less likely)
  • class of the bug (in roughly decreasing order of severity: security issue, data loss / corruption, incorrect query results, bugs that cause downtime, bugs that cause certain features to error out when they should work)
  • whether or not the first RC has gone out for a release (generally after the first RC we try to minimize changes to hard blockers only)

This particular issue sounds like it won't hit too many users (it's a relatively new feature that isn't widely adopted yet), it sounds like it did exist in the last release (0.13.0 also used an older version of datasketches), it's a moderate-high severity sort of class (incorrect query results), and we are right now on RC3 for 0.14.0. So for those reasons combined I tilt towards not including this in 0.14.0.

This thought process is a reflection of the fact that bugs are found and fixed regularly in large codebases like Druid, and the release process takes 6 days (3 days for a Druid dev list vote, 3 days for IPMC). Given that timeline it's not practical to delay releases until 100% of known bugs are fixed, or else we'd never be able release anything since we'd be constantly restarting the clock. For this reason, there are five other bugs fixed in master right now that are slated for 0.15.0 rather than 0.14.0. One other is also in the 'incorrect query results' class (#7257).

Doing a 0.14.1 before the 0.15.0 timeframe is an option that we could discuss too - the dev@druid list is probably the right place for that discussion, if you want to have it.

@fjy
Copy link
Contributor

fjy commented Apr 4, 2019

Agree with @gianm

@gianm
Copy link
Contributor

gianm commented Apr 4, 2019

By the way I don't want to minimize the level of excitement I have for datasketches in general and the contributions of your team to Druid. The level is very high - this work is definitely very interesting and appreciated. I want to make sure this sentiment isn't lost even as I am arguing a rationale for delaying this fix to the next Druid release, whether that be 0.14.1 or 0.15.0.

@AlexanderSaydakov
Copy link
Contributor Author

I understand. Is there an estimated time for 0.15.0 release? And for 0.14.1, if you decide to make it happen?

@gianm
Copy link
Contributor

gianm commented Apr 5, 2019

0.15.0 is scheduled for a code freeze at the end of April, and would be released as soon as possible after that (typically takes a few days / weeks after freeze for release to happen).

A potential 0.14.1 release and timing of it would be a community decision (just general consensus on the mailing list). I'm not sure how familiar you are with Apache community governance but starting a thread on dev@druid is enough to get that going, and it would happen if enough people are interested. If it occurs it would probably be a branch off 0.14.0-incubating rather than a release from master.

gianm pushed a commit to implydata/druid-public that referenced this pull request Apr 8, 2019
* use latest sketches-core-0.13.0

* latest release
gianm pushed a commit to implydata/druid-public that referenced this pull request Apr 14, 2019
* use latest sketches-core-0.13.0

* latest release
clintropolis pushed a commit that referenced this pull request Apr 24, 2019
* use latest sketches-core-0.13.0

* latest release
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.

None yet

7 participants