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

PHOENIX-5826 Remove guava from queryserver #33

Closed
wants to merge 2 commits into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented May 15, 2020

No description provided.

@stoty stoty requested review from joshelser and karanmehta93 and removed request for joshelser May 15, 2020 09:03
@stoty
Copy link
Contributor Author

stoty commented May 15, 2020

The non-trivial parts are:

Copying HostAndPort with minimal changes, to ensure backward compatibility in the ZK string format

The SimpleLRUCache implementation that is used instead of Guava cache.

Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

Nice little LRU Cache implementation. I think we could've gotten away with just a ConcurrentHashMap, but your cache implementation is great.

Licensing is the biggest thing we should be aware of now. I'm assuming this is the first time you're having to deal with this. The guiding requirements from the ASF are:

  1. Every artifact you distribute (src tarball, bin tarball, jars, and any/every other archive) should contain files LICENSE and NOTICE at the top-most level possible.
  2. The LICENSE contains the license text of all compatibly licensed software which is included in that artifact that is not the ALv2.
  3. NOTICE contains any information about included licensed code that an end user should be aware of. While this is technically "free form", it usually equates to just being copyright statements.
  4. L&N files should be a "sparse" as possible. That means, we should not be including things in LICENSE or NOTICE that are unnecessary (e.g. we should not include the copyright notice from the sqlalchemy code into the Java code). This results in gymnastics around multiple copies of L&N files for each "artifact" we distribute in a release.

In a nutshell, that's how licensing works. There is documentation at the ASF on this, including https://www.apache.org/legal/resolved.html, https://www.apache.org/foundation/license-faq.html, and https://infra.apache.org/apply-license.html

As far as the code goes, this change looks good :)

@@ -0,0 +1,284 @@
/*
* Copyright (C) 2011 The Guava Authors
Copy link
Member

Choose a reason for hiding this comment

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

We should ensure that a top-level NOTICE file propagates this copyright notice, as well as the containing jar.

the ASF parent pom already has convention to append entries to the NOTICE file. One example https://github.com/apache/hbase/blob/master/hbase-thrift/src/main/appended-resources/META-INF/NOTICE. The "src/main/appended-resources" is the magic set up by asf.pom.

@stoty
Copy link
Contributor Author

stoty commented May 17, 2020

Making sure that the licensing documentation is in order seems to be a bigger task than the whole Guava removal.

I have split that out into https://issues.apache.org/jira/browse/PHOENIX-5901

@stoty
Copy link
Contributor Author

stoty commented May 18, 2020

I forgot to actually push the changes. Better late than never.

@stoty
Copy link
Contributor Author

stoty commented May 18, 2020

Thank you @joshelser .
I wasn't comfortable with the comparator returning inconsistent results when the timestamp is equal, so I've changed the implementation a bit.
Could you re-check the last commit ?

@joshelser
Copy link
Member

Could you re-check the last commit ?

Looks fine. Thanks for double checking.

@asfgit asfgit closed this in baa8132 May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants