Skip to content

Conversation

@xueyumusic
Copy link
Contributor

Current fastutil is 6.5.16 and the latest fastutil version is 8.2.3. There are many bugfixes and optimization between these version. This PR upgrades fastutil to 8.2.3.

@kishoreg
Copy link
Member

Are there any backwards incompatible changes? Is it possible to test this with server running old version of fast util and broker new version of fastutil

@mayankshriv
Copy link
Contributor

mayankshriv commented Jun 17, 2019

Could you please categorize the code changes into the following buckets:

  1. Required for compiling with new library.
  2. Required for functional correctness (eg compiles with new library, but fails without this change).
  3. Code cleanup, not required for library upgrade (I see several changes to replace anonymous classes with lambda).

@xueyumusic
Copy link
Contributor Author

Hi, @kishoreg , I tested using the experiment in the doc with server running old version and controller/broker running new version, it looks the test could pass.

@xueyumusic
Copy link
Contributor Author

Hi, @mayankshriv , the required for compiling with new library is fastutil version in pom.xml.
The rest are all code clean related and not required for library upgrade except one execution statistic test result updated.
Thanks.

@mayankshriv
Copy link
Contributor

@xueyumusic Thanks for clarifying. For future, best to have separate PR's for unrelated changes. Or if the changes are small (and unrelated to the PR's title), explicitly call them out in the description.

@kishoreg Are you ok with this change?

@mayankshriv
Copy link
Contributor

@xueyumusic Also, I was curious as to what triggered this change? Did you run into an issue and identified that it was fixed in the newer version, or this is more of a proactive change? If former, please list the issues you ran into in Pinot that will be resolved with this version upgrade.

@xueyumusic
Copy link
Contributor Author

Hi, @mayankshriv when investigating select distinct query implementation I found that fastuitl version is a bit old, so submit this pr, thanks~

@kishoreg
Copy link
Member

LGTM

@mayankshriv mayankshriv merged commit 5e2a810 into apache:master Jun 28, 2019
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.

3 participants