trafodion
trafodion copied to clipboard
apache
[TRAFODION-3090]Isolated folder for trafodion-sql
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2669/
Looks good to me in principle, but I have a few comments and a few questions.
Here are a few things I wish we could do. Would you be willing to consider making some of those changes as well? Also, I am wondering what others think about these suggestions:
- I see no reason why we would want to build 3 SQL jars, one for HDP, CDH and Apache distros. Can we try to combine them into a single jar? I talked about this to @prashanth-vasudev before and he also couldn't think of a reason why we need different jars.
- Should we combine the core/sql, core/sql/lib_mgmt and possibly also core/common jars into one? We got so many different Maven projects.
- This one is more long-term: Some day we could replace the org.trafodion in our class names with org.apache.trafodion, but that will require someone with a lot of spare time and patience...
Comments/questions related to the code:
- Can you explain how this makes maintaining the code easier?
- Can you explain the naming scheme? How would a person new to the project find these source files?
- You also need to change the "eclipse" and "sql/.project" targets in core/Makefile.
Again, I see nothing wrong in principle with your change, just wanted to bring up some possible alternatives and considerations.
Hi Hans, It has been changed to one Jars with 3 soft links. I compared the Jars, true, not really needed for three.
For lib_mgmt, it's for different usage and i am not sure if it's reasonable to put it into trafodion-sql.jar. For core/common project, i didn't see it.
For maintain, it would be more easier to find and pack trafodion-sql.jar. In the past, every a period of time, i forget the name of the folder, i have to grep to find that compilation Makefile. sqlmid means sql middware. For replacing name, yes, we might have a new pull-request for it. Maybe, we can only change the group id.
Not really needed for eclipse settings, you can import in Eclipse which will be generated automatically, also for Intellij.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2685/
Hi @kevinxu021, thanks very much for combining the three jars into one, that makes me very happy :-).
About the lib_mgmt jar, that is used for stored procedures. I talked to Venkat and he said it would probably be ok to combine it with the SQL jar, but we can do that another time. Sorry about the common project, my mistake, we don't have that.
My concern with putting all of our Java sources into a folder sqlmid is the same as what you mention: Forgetting or not knowing the folder name. The original idea is: Put all the Java sources for the core/sql project into folder core/sql/src, just like for any other Maven project. That seems reasonable to me. Putting the Java code for core/sql into a folder core/sql/sqlmid - then I wonder why the classes aren't called org.trafodion.sql.sqlmid. Since nobody else is commenting on this it's probably not an issue.
About Eclipse make targets: I'm not a very experienced Eclipse user. Somehow I thought it would be a good idea to create the Eclipse .project files by running the mvn eclipse:eclipse command. See also https://issues.apache.org/jira/browse/TRAFODION-7.
See the eclipse and sql/.project make targets in file core/Makefile. Right now the eclipse target is broken, but if you move the Java files you need to update these targets as well. Also, take a look at file core/sqf/tools/sqtools.sh. This defines a function cdj that goes to the source folder for many of the SQL Java files.
Calling it middleware seems misleading as it has udr and other utilities framework. Would renaming the src directory to java-src and moving the pom.xmls to the java-src help? The directory basically says all java source for trafodion sql is in this directory.
This is a nice to have refactoring. But I would like to understand how it would make it easier to maintain which seems to be the original driving factor. It would be good to open a JIRA describing the "maintenance issue" and how this change fixes it.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2692/
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2693/
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2694/
I'd suggest we stop here, the more of the changes it has, the harder for reviewing.
+1
Thank you, this is quite a cleanup, getting rid of several unnecessary jars and many unnecessary pom files!
Have you tried building the entire package, make package-all in the top-level directory?
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2703/
Having a single trafodion-sql (non-distro specific) JAR that works across various distros. Nice effort!
Thanks. Yes, i made a package on my VM which were using jdk8. Seems last error is because of jdk7. Anyway, it has been fixed.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2704/
@svarnau Hi Steve, it seems everything is fine but it says failed. could you help?
Follow link to detailed logs: http://traf-testlogs.esgyn.com/PullReq/1577/2704/build-rh6-master-debug/Make.log and search for "[ERROR]"
I feel this PR has been overloaded with changes to multiple components. The JIRA was on the trafodion-sql.jar and now it is touching almost all pom files. 68 total files touched. I would recommend creating a separate PR for other clean-ups and optimizations.
@venkat1m, given how much additional work this would be, I wonder whether you have any specific issues with the changes. So far I do not see any unresolved issues that were raised.
The Jenkins build failed because of a change to rest pom.xml where doclint was disabled. This has no bearing on the trafodion-sql. I feel that to test, review the change and any potential side effects at run-time is difficult when we use this PR as a blanket PR for several pom.xml changes that touches several components. My 2 cents.
Thanks for your explanation, @venkat1m. If this were a brand-new, yet unreviewed PR with a single commit I would agree with you. Given the state we are in right now, I don't and recommend that we finish this PR instead of starting over.
On my side, I feel that I asked Kevin for additional fixes and he put in a lot of work to provide them. Several reviewers have reviewed his changes already. Restarting this whole process would be additional work for Kevin and the reviewers. The different changes are in different commits that you can review individually.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2709/