[GitHub] [geode] dschneider-pivotal opened pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

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/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal closed pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

[ pull request closed by dschneider-pivotal ]

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

the only reason setupReplicate has an option to add a loader is to test an error condition.
We don't need to test this for every type of region.
The reason this test has setupPartition is to see if create jdbc-mapping works with a partition. It creates a different type of async-event-queue and we wanted an end-to-end test for that use case.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

What about just using `RuntimeException` or some exception already defined in JDK?

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I think this new exception is better for a couple of reasons:
1. it is a checked exception; we want to make sure our code catches it (the compiler 
 does not force you to handle RuntimeException)
2. other things can throw the jdk exceptions. We only want this new code to handle PreconditionException. Any other exception (IllegalStateException, RuntimeException, etc.) are not expected here and should not be handled. 

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

It is better to call it `regionMapping`, instead of `newCacheElement`.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

We still call this extension "the JDBC Connector". This is the "service" for "the JDBC Connector".
We still use it to keep track of our jdbc mappings in memory. In the future, as features are added to the jdbc connector it may also keep track of other things. So I think we should keep it.
This is the "best practice" for each extension. The service feature allows the extension to have state it can find off the cache without adding new apis to the cache (like we used to do with things like the FunctionService and the QueryService).

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

Possible NPE here, if previous statement returns `null`.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

Can cache loader be applied to partitioned region?


[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I think that is okay. This is test code. If it every is null the test will fail. It should never be null. An assert could be added but in this context I think it is better to just throw an NPE exception.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

I am not sure why `CreateMappingCommandIntegrationTest` is deleted.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

done

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jchen21 commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["jchen21 (GitHub)" <gi...@apache.org>]

Shall we remove `JdbcConnectorService`? Since the `<connector-service>` has been removed from `cache.xml`?

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2836: GEODE-6010: change create jdbc-mapping to setup region and async-queue

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

it was trying to test in between a unit test and a dunit test. We had coverage for everything this test verified in the existing dunit and in the new unit test. And we found it was very hard to do real cluster config testing (which create jdbc-mapping now requires) in a single jvm. We found that colocating a locator disables cluster config in that locator. Given all of that we removed this test.

[ Full content available at: https://github.com/apache/geode/pull/2836 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org