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-5063 Create a new repo for the phoenix query server #1

Merged
merged 3 commits into from Jan 9, 2019

Conversation

Projects
None yet
3 participants
@karanmehta93
Copy link
Member

karanmehta93 commented Jan 3, 2019

@tdsilva @joshelser Please review.

@twdsilva
Copy link

twdsilva left a comment

Nice work, left some comments.


<groupId>org.apache.phoenix</groupId>
<artifactId>phoenix-queryserver</artifactId>
<version>4.15.0-HBase-1.4-SNAPSHOT</version>

This comment has been minimized.

@twdsilva

twdsilva Jan 3, 2019

Since this is now in its own repo we should use 1.0.0

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 3, 2019

Member

Phoenix-connectors started off in this way, so I followed it. But yes, we can update it. We will need to have 4 branches here as well. 1.0.0-HBase-1.4, 1.0.0-HBase-1.3, 1.0.0-HBase-1.2 and master

FYI: https://github.com/apache/phoenix-connectors/blob/4.x-HBase-1.4/presto-phoenix-shaded/pom.xml

This comment has been minimized.

@joshelser

joshelser Jan 3, 2019

Member

We will need to have 4 branches here as well

Do we need to have four branches? If we're going to make such a change like this already, I'd love to move away from such a burden.

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 3, 2019

Member

Yeah, we may not need that. I think we can put a default version of phoenix and override others at runtime for those branches. I believe such a behavior is possible with maven and builds can be setup accordingly.

This comment has been minimized.

@joshelser

joshelser Jan 3, 2019

Member

Yah, as long as we're "reasonable" with Phoenix API, we can hopefully avoid all that headache :)

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 4, 2019

Member

So, I want to keep the default right now to 4.14.1-HBase-1.4 and the branch name will be master. @joshelser @twdsilva Are you fine with this?
How are the builds being maintained currently? We need to setup new ones for this project and I will file relevant Jira's for that.

This comment has been minimized.

@joshelser

joshelser Jan 8, 2019

Member

Fine by me.

edit: Wait, are you talking about the Phoenix dependency version of the version of this project? I agree with Thomas that we should just call this new project 1.0.0.

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 8, 2019

Member

Here's what I am saying.

This project will be versioned from 1.0.0-SNAPSHOT.
The project will have a phoenix.version property which will define the versions of phoenix it will use (phoenix-core)
This value will be defaulted to 4.14.1-HBase-1.4 (Doing so will cause compilation failure since we have a patch in 4.15 (PHOENIX-4755) here, so we need to discuss this)
The compilation can be carried out with mvn clean install -DskipTests -Dphoenix.version=4.15.0-HBase-1.4-SNAPSHOT or any other version as deemed appropriate.

pom.xml Outdated
</repositories>

<scm>
<connection>scm:git:http://git-wip-us.apache.org/repos/asf/phoenix.git</connection>

This comment has been minimized.

@twdsilva

twdsilva Jan 3, 2019

these urls need to be updated

<dependency>
<groupId>org.apache.phoenix</groupId>
<artifactId>phoenix-core</artifactId>
<version>${project.version}</version>

This comment has been minimized.

@twdsilva

twdsilva Jan 3, 2019

Add a new phoenix.version to pom.xml, IMO we should depend on a released version of phoenix (not snapshot).

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 3, 2019

Member

Yes, I feel that too. We need to come up about guidelines for how much version variation can come up for compatibility purposes.

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 3, 2019

Member

@joshelser Thoughts here?

This comment has been minimized.

@joshelser

joshelser Jan 3, 2019

Member

Agree to use a release version of Phoenix (e.g. last)

I think this goes to the thread up above too -- choosing one version of Phoenix to test against. Largely, PQS should work against all versions of Phoenix, regardless of "internals". Our choice of phoenix-core to use is more about the version we will run unit tests against (which probably also means we can move this dependency to <scope>test</scope> too).

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 4, 2019

Member

I didn't quite get your point here @joshelser Can you elaborate?

This comment has been minimized.

@joshelser

joshelser Jan 8, 2019

Member

PQS doesn't have a compile-time dependency against Phoenix -- it's only necessary for a Phoenix version when we are building a tarball/packaging for PQS or when we're actually going to run PQS.

I'm thinking that we can make our lives easier by just saying "PQS unit tests run against this version of Phoenix" and running it against different versions is an exercise left to the user.

This comment has been minimized.

@karanmehta93

karanmehta93 Jan 8, 2019

Member

Yes, that makes sense. Refer to my comment for the further plan.

Show resolved Hide resolved pom.xml Outdated
@joshelser

This comment has been minimized.

Copy link
Member

joshelser commented Jan 8, 2019

A couple higher-level thoughts:

  • How do I use this? If I have Phoenix 4.14.1 installed, how do I "install PQS"?
  • Should PQS overlay the Phoenix installation or create its installation dir?
  • This needs a README.md to capture answers on the above.
@karanmehta93

This comment has been minimized.

Copy link
Member

karanmehta93 commented Jan 8, 2019

How do I use this? If I have Phoenix 4.14.1 installed, how do I "install PQS"?

We internally use the assembly plugin to create the tar ball, we can have something similar here if we want.

Should PQS overlay the Phoenix installation or create its installation dir?

Can you elaborate here?

This needs a README.md to capture answers on the above.

Sure, will add. It would include details about compilation and running for now.

@joshelser

This comment has been minimized.

Copy link
Member

joshelser commented Jan 8, 2019

Should PQS overlay the Phoenix installation or create its installation dir?
Can you elaborate here?

Essentially, is the goal to maintain a PHOENIX_HOME that looks exactly like it does today, just built from two different tarballs?

e.g. phoenix-queryserver.tgz would contain files like bin/queryserver.py, phoenix-$VERSION-queryserver.jar, and phoenix-$VERSION-thin-client.jar

@karanmehta93

This comment has been minimized.

Copy link
Member

karanmehta93 commented Jan 9, 2019

Yes, I believe that we can configure the plugin appropriately.
Good catch there, I need to get the relevant files from bin directory over here to make that happen. Will add a commit here to address that.

@karanmehta93 karanmehta93 merged commit fa8b846 into apache:4.x-HBase-1.4 Jan 9, 2019

@karanmehta93

This comment has been minimized.

Copy link
Member

karanmehta93 commented Jan 9, 2019

I pushed a commit now.
This will cause the compilation failure of the project since it points to the released version 4.14.1-HBase-1.4, which doesn't have the changes for PHOENIX-4755. The project can be successfully compiled with mvn clean install -DskipTests -Dphoenix.version=4.15.0-HBase-1.4-SNAPSHOT.

How do you feel we should address that issue? @joshelser @twdsilva

@joshelser

This comment has been minimized.

Copy link
Member

joshelser commented Jan 11, 2019

IMO, depend against 4.15.0-SNAPSHOT for now, eventually moving it to 4.15.0 when that's "official".

@twdsilva

This comment has been minimized.

Copy link

twdsilva commented Jan 12, 2019

+1 on depending on 4.15.0-SNAPSHOT.

@twdsilva

This comment has been minimized.

Copy link

twdsilva commented Jan 16, 2019

@karanmehta93 We should also just have a single branch master (instead of the current branch 4.x-HBase-1.4)

@twdsilva

This comment has been minimized.

Copy link

twdsilva commented Jan 16, 2019

@karanmehta93 Are you able to compile the project? I get the following error

[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:phoenix-queryserver:[unknown-version]
[ERROR] 'dependencies.dependency.version' for org.apache.phoenix:queryserver-client:jar is missing. @ org.apache.phoenix:queryserver:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver/pom.xml, line 121, column 17
[ERROR] Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:queryserver-client:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver-client/pom.xml
[WARNING] 'build.plugins.plugin.version' for org.apache.rat:apache-rat-plugin is missing. @ line 339, column 21
 @ 
[ERROR] The build could not read 2 projects -> [Help 1]
org.apache.maven.project.ProjectBuildingException: Some problems were encountered while processing the POMs:
[ERROR] Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:phoenix-queryserver:[unknown-version]
[ERROR] 'dependencies.dependency.version' for org.apache.phoenix:queryserver-client:jar is missing. @ org.apache.phoenix:queryserver:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver/pom.xml, line 121, column 17
[ERROR] Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:queryserver-client:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver-client/pom.xml
[WARNING] 'build.plugins.plugin.version' for org.apache.rat:apache-rat-plugin is missing. @ line 339, column 21

    at org.apache.maven.project.DefaultProjectBuilder.build (DefaultProjectBuilder.java:383)
    at org.apache.maven.graph.DefaultGraphBuilder.collectProjects (DefaultGraphBuilder.java:414)
    at org.apache.maven.graph.DefaultGraphBuilder.getProjectsForMavenReactor (DefaultGraphBuilder.java:405)
    at org.apache.maven.graph.DefaultGraphBuilder.build (DefaultGraphBuilder.java:82)
    at org.apache.maven.DefaultMaven.buildGraph (DefaultMaven.java:507)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:219)
    at org.apache.maven.DefaultMaven.doExecute (DefaultMaven.java:192)
    at org.apache.maven.DefaultMaven.execute (DefaultMaven.java:105)
    at org.apache.maven.cli.MavenCli.execute (MavenCli.java:956)
    at org.apache.maven.cli.MavenCli.doMain (MavenCli.java:288)
    at org.apache.maven.cli.MavenCli.main (MavenCli.java:192)
    at sun.reflect.NativeMethodAccessorImpl.invoke0 (Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke (NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke (DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke (Method.java:498)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launchEnhanced (Launcher.java:289)
    at org.codehaus.plexus.classworlds.launcher.Launcher.launch (Launcher.java:229)
    at org.codehaus.plexus.classworlds.launcher.Launcher.mainWithExitCode (Launcher.java:415)
    at org.codehaus.plexus.classworlds.launcher.Launcher.main (Launcher.java:356)
[ERROR]   
[ERROR]   The project org.apache.phoenix:queryserver:4.15.0-HBase-1.4-SNAPSHOT (/home/tdsilva/phoenix-queryserver/queryserver/pom.xml) has 2 errors
[ERROR]     Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:phoenix-queryserver:[unknown-version]
[ERROR]     'dependencies.dependency.version' for org.apache.phoenix:queryserver-client:jar is missing. @ org.apache.phoenix:queryserver:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver/pom.xml, line 121, column 17
[ERROR]   
[ERROR]   The project org.apache.phoenix:queryserver-client:4.15.0-HBase-1.4-SNAPSHOT (/home/tdsilva/phoenix-queryserver/queryserver-client/pom.xml) has 1 error
[ERROR]     Invalid packaging for parent POM org.apache.phoenix:phoenix-queryserver:[unknown-version], must be "pom" but is "jar" @ org.apache.phoenix:queryserver-client:[unknown-version], /home/tdsilva/phoenix-queryserver/queryserver-client/pom.xml
[ERROR] 
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
@karanmehta93

This comment has been minimized.

Copy link
Member

karanmehta93 commented Jan 16, 2019

Didn't realize that I accidentally committed it to the apache repo. I have reverted it and will push it once everything is done and PR review is completed. @twdsilva @joshelser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment