You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "dschneider-pivotal (GitHub)" <gi...@apache.org> on 2018/10/18 00:36:37 UTC
[GitHub] [geode] dschneider-pivotal opened pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
This pull request is still a work in progress.
The old jdbc-connection gfsh commands still need to be removed.
The new create jndi-binding --type=POOLED needs some additional work.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Might want to change this to:
```
if (statement == null && connection != null) {
```
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
My understanding of line 114 was that this XmlElement annotation applied to the next line which was: protected List<ConnectorService.Connection> connection;
But we deleted the "connection" line.
Originally we left line 114 in and the code failed to compile.
So I'm going to resolve this. I don't understand these xml annotations well so let me know if I have this wrong.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
This will be fixed on another pull request. Thanks for catching it.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Looks like we still have connector-service in our jdbc xml (but we deleted the namespace). Should we keep this test and just remove the lines for connection or should jdbc xml be deleted?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] agingade commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "agingade (GitHub)" <gi...@apache.org>.
does "--name" implies connection name? If its then change it to "connection-name"
The JdbcConnectorException message:
"JDBC connection with name " + connectionName + " not found. Create the connection with the gfsh command 'create jndi-binding'"
Sounds like connection-name is part of jndi-binding
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] agingade commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "agingade (GitHub)" <gi...@apache.org>.
What is "--connection" is referring to here? Is it supposed to be jndi binding name?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
Yes, thanks for catching this. We intended to clean it up but forgot to do so.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
thanks, will do
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
(Looks like my comments got reordered by github so further down might be up)
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
will move it to the end of the class
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
the connector-service will go away in another story related to the nested region-mapping
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
It is the jndi-binding's name. We have another story to figure out how we want to refer to the "jndi-binding". Once we do, then in that story we will convert the various things like this to use that name.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Should this use a Logger instead of System.out?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Note: deleted jdbc xml namespace.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-amurmann commented on issue #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "pivotal-amurmann (GitHub)" <gi...@apache.org>.
@dschneider-pivotal I am curious about the change to the `--type` option. Might I have previously passed a `CONNECTION_POOLED_DATASOURCE_CLASS` option without setting the `--type` option? If so, this is a breaking change, isn't it?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
will do
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on issue #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
@pivotal-amurmann yes it is a breaking change and this was called out to the PM (Swapnil and now Diane has been told about it). The thought was that this particular type was not being used by anyone. When changing this to the new behavior I do remember seeing something that would have been broken if anyone tried to use the old behavior which reassured me that no one was using it (because they would have reported that issue).
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on issue #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
The one failure in integrationTest was a known issue (has a jira ticket) and was not related to this pr.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] agingade commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "agingade (GitHub)" <gi...@apache.org>.
Can we remove the reference from integration test. Its fine to have duplicate code.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
The namespace we deleted had to do with the "connection" which was deleted not the connector-service. I could not find were we deleted processing for connector-service; it should have just been for connection. Let me know if this should not have been resolved.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
will do
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Might want to change this to:
```
if (statement ==null && connection != null) {
```
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Move constant to top of class.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
I think it is for external javadocs. Anytime we add new external javadocs they need to be added to this file
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
This is test code so I think system out is acceptable. I remember a discussion about forcing us not to use the logger in test code. This code does rethrow so I think it is okay. Let me know if we decided to force tests to use the logger instead of System.out.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
We still have jdbc-1.0.xsd with connector-service element. However, we deleted the namespace and further down in this review we deleted processing for connector-service.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
The default is "{}" so you can just delete close() {}.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
Yes, this will be addressed in the other story about what to call the jndi-binding.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2650: GEODE-5861:
change jdbc connector to use jndi binding
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Are `server1` and `server2` needed for `invokeInEveryMember`?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
done
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal closed pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
[ pull request closed by dschneider-pivotal ]
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
Yes we still have the connector-service element. We only removed the connection sub-element of it.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Here's a reference to the product code for element of connection-service (or is it connector-service?).
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2650: GEODE-5861:
change jdbc connector to use jndi binding
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Just wonder what is the usage of `assembly_content.txt`?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
I would sink this inner-interface to the bottom of the outer-class.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Spotless did a number on the javadoc here. Might want to delete some line-feeds.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
we still have connector-service as explained. But this particular test was only testing the nested "connection" element that was removed. In this same class we kept the test for the nested region-mapping.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] dschneider-pivotal commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.
for unit test access
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2650:
GEODE-5861: change jdbc connector to use jndi binding
Posted by "kirklund (GitHub)" <gi...@apache.org>.
You can just delete empty no-arg constructors like this. Looks weird after spotless mangles it like this.
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jchen21 commented on pull request #2650: GEODE-5861:
change jdbc connector to use jndi binding
Posted by "jchen21 (GitHub)" <gi...@apache.org>.
Is there any reason that `getConnection` and `getDataSource` stay at package level? Not `public`?
[ Full content available at: https://github.com/apache/geode/pull/2650 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org