You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/17 14:54:33 UTC
[GitHub] [geode] masaki-yamakawa opened a new pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
masaki-yamakawa opened a new pull request #5637:
URL: https://github.com/apache/geode/pull/5637
Use the default mapping when using the Simple JDBC Connector in cache.xml and without the field-mapping tag
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:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [X] 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, check Concourse 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-807967415
This PR has been open for some while with no recent activity. Please keep Geode tidy by closing PRs you're not actively using.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-758854931
@masaki-yamakawa Once you are ready, please click the re-request review button on the reviewers section.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153415
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
As you mentioned, I have moved some methods.
In addition, I have reduced the the particle size of the test.
The corresponding test for `executionFuncationGivenNonPdxUsesReflectionBasedAutoSerializer` is `JdbcConnectorServiceTest.getPdxTypeForClassSucceedsWithGivenNonPdxUsesReflectionBasedAutoSerializer`.
This test verify the PdxType. And then, the PdxType is specified to `createDefaultFieldMapping` to create the FieldMapping.
createDefaultFieldMapping is tested with `JdbcConnectorServiceTest.createDefaultFieldMappingXXX`, which verify the FieldMapping at this level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r554270893
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1142,7 +1142,7 @@ public void createMappingWithExistingQueueFails() {
+ " must not already exist.");
}
- private static class Employee implements PdxSerializable {
+ public static class Employee implements PdxSerializable {
Review comment:
On the surface, it seems a trivial change from `private` to `public`, in order to fix the exception. However, under the hood, it is doing unnecessary serialization and deserialization of the `Employee` class. Because `getPdxTypeForClass()`, `generatePdxTypeForClass() ` and `createInstance()` are moved from `CreateMappingPreconditionCheckFunction` to `JdbcConnectorServiceImpl`, which is probably because you want to use these methods in `RegionMappingConfiguration`.
I would recommend keeping `getPdxTypeForClass()`, `generatePdxTypeForClass() ` and `createInstance()` in `CreateMappingPreconditionCheckFunction`. And in `RegionMappingConfiguration`, implement similar methods. Though it looks like duplicating the code, it does avoid unnecessary serialization and deserialization of user defined classes like `Employee`, which is more important.
And I don't think adding more methods to `JdbcConnectorService` is necessary. e.g. `createDefaultFieldMapping()` is something you want to automatically achieve for the users, so it is not necessary be exposed as an API. Just put it as a private method in `RegionMappingConfiguration` should be good. The same reason applies for `getTableMetaDataView()` and `getPdxTypeForClass`. Basically, keep `JdbcConnectorService` unchanged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] onichols-pivotal merged pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
onichols-pivotal merged pull request #5637:
URL: https://github.com/apache/geode/pull/5637
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-867037067
@masaki-yamakawa
Thanks for your contribution to Geode. Appreciate your patience in getting this done. It was a tricky one as it was adding a new way to build field mapping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153350
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/xml/RegionMappingConfiguration.java
##########
@@ -62,4 +77,17 @@ private void createRegionMapping(JdbcConnectorService service,
throw new InternalGemFireException(e);
}
}
+
+ private Class<?> loadPdxClass(String className) {
+ try {
+ return loadClass(className);
Review comment:
I have fixed this part.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-770988583
@agingade
Thanks for your comment.
I am using "Peer-to-Peer Configuration" to build the application. Specifically, it is an application that integrates Tomcat and Geode.
It achieves high-performance by managing data in the App heap (Geode). Then, the data in Geode is persisted to RDB asynchronously.
I understand that gfsh and cluster configuration Import/Export can be used in this configuration. However, I would like to avoid this method because of the complication in operation and the risk of mistakes.
Thus I want a configuration file that reads the region definition when the App is launched.
Nowadays, yml, json and annotations are commonly used.
Geode does not support these, so I chose to use XML.
* I know SpringData For ApacheGeode, but I don't use it because I want to use Geode's native API.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r520637876
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
Review comment:
Thank you.
I would like to fix it as you suggested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-901543704
Thanks @jinmeiliao @agingade @jchen21 for review.
Is this PR OK to merge?
If there's anything I shouldo do, please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-859218073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-720867380
@masaki-yamakawa I apologize for the delay in reviewing this change.
@jchen21 and @dschneider-pivotal would you be in a position to review this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r555133247
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
I removed `GfshJdbcMappingIntegrationTest` because I no longer need to impliment it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-764638520
@agingade This PR is ready to be reviewed. Please review it when you're available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r554270893
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1142,7 +1142,7 @@ public void createMappingWithExistingQueueFails() {
+ " must not already exist.");
}
- private static class Employee implements PdxSerializable {
+ public static class Employee implements PdxSerializable {
Review comment:
On the surface, it seems a trivial change from `private` to `public`, in order to fix the exception. However, under the hood, it is doing unnecessary serialization and deserialization of the `Employee` class. Because `getPdxTypeForClass()`, `generatePdxTypeForClass() ` and `createInstance()` are moved from `CreateMappingPreconditionCheckFunction` to `JdbcConnectorServiceImpl`, which is probably because you want to use these methods in `RegionMappingConfiguration`.
I would recommend keeping `getPdxTypeForClass()`, `generatePdxTypeForClass() ` and `createInstance()` in `CreateMappingPreconditionCheckFunction`. And in `RegionMappingConfiguration`, implement similar methods. Though it looks like duplicating the code, it does avoid unnecessary serialization and deserialization of user defined classes like `Employee`, which is more important.
And I don't think adding more methods to `JdbcConnectorService` is necessary. e.g. `createDefaultFieldMapping()` is something you want to automatically achieve for the users, so it is not necessary be exposed as an API. Just put it as a private method in `RegionMappingConfiguration` should be good. The same reason applies for `getTableMetaDataView()` and `getPdxTypeForClass`. Basically, keep `JdbcConnectorService` unchanged.
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
If you want to use `gfsh` and verify the field mapping, `CreateMappingCommandDUnitTest` has some good examples which use `GfshCommandRule` (not `GfshRule` though). You can see if you would like add some more tests there. Typically, a DUnit is used for testing that involves `gfsh` commands. In the DUnit, you create a cluster with locator and server(s) and execute the `gfsh` commands.
Using the functions that implement a specific `gfsh` command in integration tests is discouraged. That is not what an integration test should do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-758029719
@jchen21 Thank you for your review. I have fixed the code following your suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r523884148
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
Review comment:
I have fixed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r523884193
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
+ }
+ return getTableMetaDataView(regionMapping, dataSource);
+ }
+
+ @Override
+ public List<FieldMapping> createDefaultFieldMapping(RegionMapping regionMapping,
+ PdxType pdxType) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when creating mapping \"" + regionMapping.getRegionName() + "\"");
Review comment:
I have fixed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r520637605
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
OK, I will try to implement the test using gfsh commands.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-854120507
@masaki-yamakawa
My worry is abandoning the xml support in near future. If you insist to go with this approach, its fine with me.
One change that could help code maintainability is moving your changes (which is duplicate of the code/logic from CreateMappingPreconditionCheckFunction) to RegionMapping as helper methods and then accessing it from both place.
E.g.:
if (mapping.getFieldMappings().isEmpty() && mapping.getPdxName != null) {
mapping.createMappingUsingPdx();
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r520038736
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceTest.java
##########
@@ -216,4 +249,213 @@ public void validateMappingSucceedsWithCompositeKeys() {
when(mapping.getIds()).thenReturn(KEY_COLUMN_NAME + "," + COMPOSITE_KEY_COLUMN_NAME);
service.validateMapping(mapping);
}
+
+ @Test
+ public void getTableMetaDataViewSucceeds() {
+ TableMetaDataView result = service.getTableMetaDataView(mapping);
+ assertThat(result).isEqualTo(view);
+ verify(manager).getTableMetaDataView(connection, mapping);
+ }
+
+ @Test
+ public void getTableMetaDataViewThrowsExceptionWhenDataSourceDoesNotExist() {
+ doReturn(null).when(service).getDataSource(DATA_SOURCE_NAME);
+ Throwable throwable = catchThrowable(() -> service.getTableMetaDataView(mapping));
+ assertThat(throwable).isInstanceOf(JdbcConnectorException.class).hasMessageContaining(
+ String.format("No datasource \"%s\" found when getting table meta data \"%s\"",
+ mapping.getDataSourceName(), mapping.getRegionName()));
+ }
+
+ @Test
+ public void getTableMetaDataViewThrowsExceptionWhenGetConnectionHasSqlException()
+ throws SQLException {
+ when(dataSource.getConnection()).thenThrow(SQLException.class);
Review comment:
Minor pick. I would recommend some error message for the `SQLException`, e.g.
```
String reason = "connection failed";
when(dataSource.getConnection()).thenThrow(new SQLException(reason));
```
The the error message in the next few lines would be `Exception thrown while connecting to datasource \"dataSource\": connection failed`, instead of `Exception thrown while connecting to datasource \"dataSource\": null`
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
This is an integration test, which tests the behavior of gfsh commands from a user's point view. I don't think this test should use internal functions of specific gfsh command's implementation. The users should not worry about the gfsh implementation.
And I don't recommend using a lot of `System.out.println` in the tests. Is that for debugging purpose?
If you have to use internal functions, I would recommend testing them in a unit test, or some other integration test or dunit that specifically test a specific gfsh command. You said you use the internal functions because some parts of `GfshRule` can not verify in case of errors. Can you give a specific example?
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -172,16 +168,6 @@ public void executeFunctionThrowsIfDataSourceDoesNotExist() {
+ DATA_SOURCE_NAME + "'.");
}
- @Test
Review comment:
👍
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.internal.cache.InternalCache;
+
+public class CacheXmlJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
Review comment:
👍
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
Review comment:
I was trying to say:
```
if (dataSource == null) {
throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName());
}
```
If you would like to provide more information,
```
if (dataSource == null) {
throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ "\" found when getting table meta data \"" + regionMapping.getTableName() + "\"");
}
```
Note that it is `regionMapping.getTableName()`, not `regionMapping.getRegionName()`.
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
I understand that. However, none of the `JdbcConnectorServiceTest.createDefaultFieldMapping*` uses `ReflectionBasedAutoSerializer`. Although those tests do verify the field mapping. I expect there is some test that uses `ReflectionBasedAutoSerializer` and verifies the field mapping as well, like the test deleted below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r520101096
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/xml/RegionMappingConfiguration.java
##########
@@ -62,4 +77,17 @@ private void createRegionMapping(JdbcConnectorService service,
throw new InternalGemFireException(e);
}
}
+
+ private Class<?> loadPdxClass(String className) {
+ try {
+ return loadClass(className);
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-770014534
> @agingade This PR is ready to be reviewed. Please review it when you're available.
@masaki-yamakawa
Participants in team discussion: @jchen21 @dschneider-pivotal
Thanks for your PR. Sorry for delay in responding to this.
We had a team discussion around this PR as it deflects from the way Geode configuration is done (the recommended way). In the past Geode/GemFire was built with cache.xml as a way to configure the system; based on its drawback and keep the overall cluster configuration easier, it was decided to adopt/build Cluster Configuration Service with gfsh as the tool to accomplish it (currently there is work getting done to support the same through new management APIs). As the Cluster Configuration Service got matured, the use of xml is gradually getting deprecated.
The change proposed with this ticket/PR feels like going back to the cache.xml way of configuring the system. Can you please let us know the reason behind not using the cluster configuration service and going with cache.xml; we would like to understand more about the usecase/requirement and see if there are any other better options.
Also, you can achieve what you are looking for, by exporting the cluster configuration info (once defined using gfsh - which is an xml output) and then importing it to any test/dev/prod system. Can this suffice your requirement?
On the other note, based on the PR review, it expects the user class to be in the server side classpath; currently this is not a requirement while using PDX type/object; one of the main functionality of PDX is to support data stores without needing a class definition on the server. The product takes care of this internally when one configures the jdbc mapping automatically.
Please let us know your thoughts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r523884570
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
I have added mappingSuccessWhenFieldMappingsAreOmittedWithNonSerializedClass test method that uses ReflectionBasedAutoSerializer to JdbcMappingIntegrationTest
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-754708386
@jchen21
I have fixed all suggestions and added comments.
Please re-review when you're available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r555132569
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1142,7 +1142,7 @@ public void createMappingWithExistingQueueFails() {
+ " must not already exist.");
}
- private static class Employee implements PdxSerializable {
+ public static class Employee implements PdxSerializable {
Review comment:
Thank you for your detailed explanation. I have reverted the `CreateMappingPreconditionCheckFunction` and `JdbcConnectorService` to the original source code. And I have implemented similar methods in `RegionMappingConfiguration`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-859177995
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153030
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.internal.cache.InternalCache;
+
+public class CacheXmlJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
Review comment:
When expanding Java system properties like `<jndi-binding ... connection-url="${TestDataSourceUrl}">` in cache.xml and the connection string contains '?' such as `jdbc:mysql://localhost:3306?user=root&useSSL=false`, if the connection string is not escaped, an error will occur when connecting to the database.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153180
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1142,7 +1142,7 @@ public void createMappingWithExistingQueueFails() {
+ " must not already exist.");
}
- private static class Employee implements PdxSerializable {
+ public static class Employee implements PdxSerializable {
Review comment:
In the test, `CreatemappingPreconditionCheckFunction` is called and executed on several JVMs(Distributed members).
In this Function, PDX class such as Employee is instantiated, so I need to be able to access this class even if it runs on another JVM.
If it is in private scope, the following exceptions will be thrown
```
[error 2020/11/07 12:08:19.411 JST <Function Execution Processor2> tid=69] Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingCommandDUnitTest$IdAndName because it did not have a public zero arg constructor. Details: java.lang.IllegalAccessException: Class org.apache.geode.connectors.jdbc.internal.JdbcConnectorServiceImpl can not access a member of class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingCommandDUnitTest$IdAndName with modifiers "public"
org.apache.geode.connectors.jdbc.JdbcConnectorException: Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingCommandDUnitTest$IdAndName because it did not have a public zero arg constructor. Details: java.lang.IllegalAccessException: Class org.apache.geode.connectors.jdbc.internal.JdbcConnectorServiceImpl can not access a member of class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingCommandDUnitTest$IdAndName with modifiers "public"
at org.apache.geode.connectors.jdbc.internal.JdbcConnectorServiceImpl.createInstance(JdbcConnectorServiceImpl.java:361)
at org.apache.geode.connectors.jdbc.internal.JdbcConnectorServiceImpl.generatePdxTypeForClass(JdbcConnectorServiceImpl.java:334)
at org.apache.geode.connectors.jdbc.internal.JdbcConnectorServiceImpl.getPdxTypeForClass(JdbcConnectorServiceImpl.java:320)
at org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction.executeFunction(CreateMappingPreconditionCheckFunction.java:74)
at org.apache.geode.management.cli.CliFunction.execute(CliFunction.java:37)
...
```
Other similar tests like CreateMapingCommandForProxyRegionDUnitTest are also public scope.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r517719407
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1142,7 +1142,7 @@ public void createMappingWithExistingQueueFails() {
+ " must not already exist.");
}
- private static class Employee implements PdxSerializable {
+ public static class Employee implements PdxSerializable {
Review comment:
Why this class has to be `public`?
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+
+import org.junit.Rule;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.internal.cache.InternalCache;
+
+public class CacheXmlJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
Review comment:
Is this replacement of `&` necessary?
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
+ }
+ return getTableMetaDataView(regionMapping, dataSource);
+ }
+
+ @Override
+ public List<FieldMapping> createDefaultFieldMapping(RegionMapping regionMapping,
+ PdxType pdxType) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when creating mapping \"" + regionMapping.getRegionName() + "\"");
Review comment:
The data source has nothing to do with table metadata or region name. I recommend removing this line of error message.
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -172,16 +168,6 @@ public void executeFunctionThrowsIfDataSourceDoesNotExist() {
+ DATA_SOURCE_NAME + "'.");
}
- @Test
Review comment:
Why this test is removed?
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
To create region, data store and mapping etc, I would recommend using `GfshRule` and execute gfsh commands, instead of calling the internal functions that implement the gfsh commands.
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1234,7 +1234,7 @@ void setRefid(long refid) {
}
}
- private static class IdAndName implements PdxSerializable {
+ public static class IdAndName implements PdxSerializable {
Review comment:
Why this class has to be public?
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/xml/RegionMappingConfiguration.java
##########
@@ -62,4 +77,17 @@ private void createRegionMapping(JdbcConnectorService service,
throw new InternalGemFireException(e);
}
}
+
+ private Class<?> loadPdxClass(String className) {
+ try {
+ return loadClass(className);
Review comment:
Since its implementation is just one line, why not inline it?
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1185,7 +1185,7 @@ public void fromData(PdxReader reader) {
}
}
- private static class EmployeeNumeric implements PdxSerializerObject {
+ public static class EmployeeNumeric implements PdxSerializerObject {
Review comment:
Why this class has to be public?
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
Some functions of `CreateMappingPreconditionCheckFunction` are moved to `JdbcConnectorServiceImpl`. So if you remove this test here, there should be similar test coverage added in `JdbcConnectorServiceTest`. In `JdbcConnectorServiceTest` there are some tests that test `ReflectionBasedAutoSerializer`. However, those tests don't verify `FieldMapping` like this test.
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
Review comment:
The data source has nothing to do with table metadata or region name. I recommend removing this line of error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-855175045
@agingade
Thank you for your concern. I have no problem.
I agree with your suggestion. And I had implemented it that way in a previous change.
But the review resulted in the current implementation.
https://github.com/apache/geode/pull/5637#discussion_r554270893
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153095
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
I understand that it is recommended to use GfshRule.
With this change, I would like to achieve the same behavior in gfsh command and cache.xml.
So I have implimented `JdbcMappingIntegrationTest` class to standardize tests.
I have used the internal functions because some parts of GfshRule can not verify in case of errors.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153312
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceImpl.java
##########
@@ -210,4 +224,152 @@ private TableMetaDataView getTableMetaDataView(RegionMapping regionMapping,
+ regionMapping.getDataSourceName() + "\": ", ex);
}
}
+
+ @Override
+ public TableMetaDataView getTableMetaDataView(RegionMapping regionMapping) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
Review comment:
The data source is used to get the table metadata.
I would like to confirm your suggestion.
Your suggestion is the following, right?
```
if (dataSource == null) {
// throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
// + "\" found when getting table meta data \"" + regionMapping.getRegionName() + "\"");
throw new JdbcConnectorException();
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r520638425
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
I will add the test using the `ReflectionBasedAutoSerializer` and verify the field mapping.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-809024735
@agingade
This PR is to make the current behavior of using cache.xml closer to the behavior of using gfsh.
I would like to consider it separately from the discussion of whether to use cache.xml in the future.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-757076684
@masaki-yamakawa I have reviewed your code again. Please see the comments in the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153380
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -172,16 +168,6 @@ public void executeFunctionThrowsIfDataSourceDoesNotExist() {
+ DATA_SOURCE_NAME + "'.");
}
- @Test
Review comment:
I changed `CreateMappingPreconditionCheckFunction` class to use `JdbcConnectorService.getTableMetaDataView(regionMapping)`.
This test covered by `JdbcConnectorServiceTest.getTableMetaDataViewThrowsExceptionWhenGetConnectionHasSqlException`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r523884751
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
I tried to implement the test using GfshRule, but I couldn't do it because I use `cache.getService(JdbcConnectorService.class)` to get the implementation of `JdbcConnectorService` to verify FieldMapping, and GfshRule does not allow it.
And I would like to make the condition setting part and the verification part of `CacheXmlJdbcMappingIntegrationTest` and` GfshXmlJdbcMappingIntegrationTest` common.
So I plan one of the following.
1. Change the `GfshJdbcMappingIntegrationTest` to `FunctionJdbcMappingIntegrationTest`.
2. Remove `GfshJdbcMappingIntegrationTest`. (I can use the `describe jdbc-mapping` command to verify it, but I don't think I should do that because it would reduce maintainability and make it difficult to commonize tests.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-722021982
Reviewing the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153248
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1185,7 +1185,7 @@ public void fromData(PdxReader reader) {
}
}
- private static class EmployeeNumeric implements PdxSerializerObject {
+ public static class EmployeeNumeric implements PdxSerializerObject {
Review comment:
The reason is the same as previous one.
##########
File path: geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingCommandDUnitTest.java
##########
@@ -1234,7 +1234,7 @@ void setRefid(long refid) {
}
}
- private static class IdAndName implements PdxSerializable {
+ public static class IdAndName implements PdxSerializable {
Review comment:
The reason is the same as previous one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r523890360
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/JdbcConnectorServiceTest.java
##########
@@ -216,4 +249,213 @@ public void validateMappingSucceedsWithCompositeKeys() {
when(mapping.getIds()).thenReturn(KEY_COLUMN_NAME + "," + COMPOSITE_KEY_COLUMN_NAME);
service.validateMapping(mapping);
}
+
+ @Test
+ public void getTableMetaDataViewSucceeds() {
+ TableMetaDataView result = service.getTableMetaDataView(mapping);
+ assertThat(result).isEqualTo(view);
+ verify(manager).getTableMetaDataView(connection, mapping);
+ }
+
+ @Test
+ public void getTableMetaDataViewThrowsExceptionWhenDataSourceDoesNotExist() {
+ doReturn(null).when(service).getDataSource(DATA_SOURCE_NAME);
+ Throwable throwable = catchThrowable(() -> service.getTableMetaDataView(mapping));
+ assertThat(throwable).isInstanceOf(JdbcConnectorException.class).hasMessageContaining(
+ String.format("No datasource \"%s\" found when getting table meta data \"%s\"",
+ mapping.getDataSourceName(), mapping.getRegionName()));
+ }
+
+ @Test
+ public void getTableMetaDataViewThrowsExceptionWhenGetConnectionHasSqlException()
+ throws SQLException {
+ when(dataSource.getConnection()).thenThrow(SQLException.class);
Review comment:
Thank you for your recommendation. I have fixed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-850969848
@agingade
I would like to take this PR to the next step.
Is there anything I can do?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r519153415
##########
File path: geode-connectors/src/test/java/org/apache/geode/connectors/jdbc/internal/cli/CreateMappingPreconditionCheckFunctionTest.java
##########
@@ -306,45 +292,10 @@ public void executeFunctionThrowsGivenPdxSerializableWithNoZeroArgConstructor()
"Could not generate a PdxType for the class org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg because it did not have a public zero arg constructor. Details: java.lang.NoSuchMethodException: org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunctionTest$PdxClassDummyNoZeroArg.<init>()");
}
- @Test
Review comment:
As you mentioned, I have moved some methods.
In addition, I have reduced the the particle size of the test.
The corresponding test for `executionFuncationGivenNonPdxUsesReflectionBasedAutoSerializer` is `JdbcConnectorServiceTest.getPdxTypepeForClassSucceedsWithGive nonPdxUsesReflectionBasedAutoSerializer`.
This test verify the PdxType. And then, the PdxType is specified to `createDefaultFieldMapping` to create the FieldMapping.
createDefaultFieldMapping is tested with `JdbcConnectorServiceTest.createDefaultFieldMappingXXX`, which verify the FieldMapping at this level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r554273414
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/GfshJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,208 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import java.lang.reflect.Constructor;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.cache.configuration.JndiBindingsType;
+import org.apache.geode.cache.configuration.RegionConfig;
+import org.apache.geode.cache.execute.Function;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingFunction;
+import org.apache.geode.connectors.jdbc.internal.cli.CreateMappingPreconditionCheckFunction;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.management.configuration.RegionType;
+import org.apache.geode.management.internal.cli.commands.CreateJndiBindingCommand;
+import org.apache.geode.management.internal.cli.functions.CreateJndiBindingFunction;
+import org.apache.geode.management.internal.cli.functions.CreateRegionFunctionArgs;
+import org.apache.geode.management.internal.cli.functions.RegionCreateFunction;
+import org.apache.geode.management.internal.configuration.converters.RegionConverter;
+import org.apache.geode.management.internal.functions.CliFunctionResult;
+import org.apache.geode.management.internal.util.ManagementUtils;
+
+public class GfshJdbcMappingIntegrationTest extends JdbcMappingIntegrationTest {
+
+ @Override
+ protected InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0").create();
+ Set<DistributedMember> targetMembers = findMembers(cache, null, null);
+
+ CliFunctionResult createRegionFuncResult = executeCreateRegionFunction(targetMembers);
Review comment:
If you want to use `gfsh` and verify the field mapping, `CreateMappingCommandDUnitTest` has some good examples which use `GfshCommandRule` (not `GfshRule` though). You can see if you would like add some more tests there. Typically, a DUnit is used for testing that involves `gfsh` commands. In the DUnit, you create a cluster with locator and server(s) and execute the `gfsh` commands.
Using the functions that implement a specific `gfsh` command in integration tests is discouraged. That is not what an integration test should do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-757076684
@masaki-yamakawa I have reviewed your code again. Please see the comments in the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-723419059
@jchen21 Thank you for your review.
I have fixed some of the source code and added comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] masaki-yamakawa commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
masaki-yamakawa commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-764638520
@agingade This PR is ready to be reviewed. Please review it when you're available.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5637:
URL: https://github.com/apache/geode/pull/5637#discussion_r555418639
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+
+import java.net.URL;
+import java.sql.Connection;
+import java.sql.JDBCType;
+import java.sql.Statement;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.connectors.jdbc.internal.JdbcConnectorService;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.connectors.jdbc.test.junit.rules.DatabaseConnectionRule;
+import org.apache.geode.connectors.jdbc.test.junit.rules.MySqlConnectionRule;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.jndi.JNDIInvoker;
+import org.apache.geode.pdx.FieldType;
+import org.apache.geode.pdx.internal.AutoSerializableManager;
+
+public class CacheXmlJdbcMappingIntegrationTest {
+
+ private static final URL COMPOSE_RESOURCE_PATH =
+ CacheXmlJdbcMappingIntegrationTest.class.getResource("mysql.yml");
+ protected static final String DATA_SOURCE_NAME = "TestDataSource";
+ protected static final String DB_NAME = "test";
+ protected static final String REGION_TABLE_NAME = "employees";
+ protected static final String REGION_NAME = "Region1";
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ private Connection connection;
+ private Statement statement;
+ private InternalCache cache;
+
+ @ClassRule
+ public static DatabaseConnectionRule dbRule = new MySqlConnectionRule.Builder()
+ .file(COMPOSE_RESOURCE_PATH.getPath()).serviceName("db").port(3306).database(DB_NAME).build();
+
+ @Before
+ public void setUp() throws Exception {
+ System.setProperty(AutoSerializableManager.NO_HARDCODED_EXCLUDES_PARAM, "true");
+ connection = dbRule.getConnection();
+ statement = connection.createStatement();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ JNDIInvoker.unMapDatasource(DATA_SOURCE_NAME);
+
+ if (cache != null) {
+ cache.close();
+ }
+
+ if (statement == null) {
+ statement = connection.createStatement();
+ }
+ statement.execute("Drop table IF EXISTS " + REGION_TABLE_NAME);
+ statement.close();
+
+ if (connection != null) {
+ connection.close();
+ }
+ }
+
+ private InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
+ System.setProperty("TestDataSourceUrl", url);
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithNonSerializedClass(
+ String cacheXmlTestName) throws Exception {
+ return createCacheAndCreateJdbcMapping(cacheXmlTestName);
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithWrongDataSource(
+ String cacheXmlTestName) throws Exception {
+ System.setProperty("TestDataSourceUrl", "jdbc:mysql://localhost/test");
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithWrongPdxName(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
+ System.setProperty("TestDataSourceUrl", url);
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private String getXmlFileForTest(String testName) {
+ return createTempFileFromResource(getClass(),
+ getClassSimpleName() + "." + testName + ".cache.xml").getAbsolutePath();
+ }
+
+ private String getClassSimpleName() {
+ return getClass().getSimpleName();
+ }
+
+ private void createEmployeeTable() throws Exception {
+ statement.execute("Create Table " + REGION_TABLE_NAME
+ + " (id varchar(10) primary key not null, name varchar(10), age int)");
+ }
+
+ private void createEmployeeTableWithColumnNamesWithUnderscores() throws Exception {
+ statement.execute("Create Table " + REGION_TABLE_NAME
+ + " (id varchar(10) primary key not null, _name varchar(10), _age int)");
+ }
+
+ private List<FieldMapping> getEmployeeTableFieldMappings() {
+ List<FieldMapping> fieldMappings = Arrays.asList(
+ new FieldMapping("id", FieldType.STRING.name(), "id", JDBCType.VARCHAR.name(), false),
+ new FieldMapping("name", FieldType.STRING.name(), "name", JDBCType.VARCHAR.name(), true),
+ new FieldMapping("age", FieldType.INT.name(), "age", JDBCType.INTEGER.name(), true));
+ return fieldMappings;
+ }
+
+ private List<FieldMapping> getEmployeeTableColumnNameWithUnderscoresFieldMappings() {
+ List<FieldMapping> fieldMappings = Arrays.asList(
+ new FieldMapping("id", FieldType.STRING.name(), "id", JDBCType.VARCHAR.name(), false),
+ new FieldMapping("name", FieldType.STRING.name(), "_name", JDBCType.VARCHAR.name(), true),
+ new FieldMapping("age", FieldType.INT.name(), "_age", JDBCType.INTEGER.name(), true));
+ return fieldMappings;
+ }
+
+ @Test
+ public void mappingSuccessWhenFieldMappingsAreExists() throws Exception {
+ createEmployeeTable();
+
+ cache = createCacheAndCreateJdbcMapping("FieldMappings");
+ JdbcConnectorService service = cache.getService(JdbcConnectorService.class);
+
+ RegionMapping mapping = service.getMappingForRegion(REGION_NAME);
+ assertThat(mapping.getDataSourceName()).isEqualTo(DATA_SOURCE_NAME);
+ assertThat(mapping.getTableName()).isEqualTo(REGION_TABLE_NAME);
+ assertThat(mapping.getRegionName()).isEqualTo(REGION_NAME);
+ assertThat(mapping.getPdxName()).isEqualTo(Employee.class.getName());
+ assertThat(mapping.getIds()).isEqualTo("id");
+ assertThat(mapping.getFieldMappings().size()).isEqualTo(3);
+ assertThat(mapping.getFieldMappings()).containsAll(getEmployeeTableFieldMappings());
+ }
+
+ @Test
+ public void mappingSuccessWhenFieldMappingsAreOmitted() throws Exception {
+ createEmployeeTable();
+
+ cache = createCacheAndCreateJdbcMapping("NoFieldMappings");
+ JdbcConnectorService service = cache.getService(JdbcConnectorService.class);
+
+ RegionMapping mapping = service.getMappingForRegion(REGION_NAME);
+ assertThat(mapping.getDataSourceName()).isEqualTo(DATA_SOURCE_NAME);
+ assertThat(mapping.getTableName()).isEqualTo(REGION_TABLE_NAME);
+ assertThat(mapping.getRegionName()).isEqualTo(REGION_NAME);
+ assertThat(mapping.getPdxName()).isEqualTo(Employee.class.getName());
+ assertThat(mapping.getIds()).isEqualTo("id");
+ assertThat(mapping.getFieldMappings().size()).isEqualTo(3);
+ assertThat(mapping.getFieldMappings()).containsAll(getEmployeeTableFieldMappings());
+ }
+
+ @Test
+ public void mappingSuccessWhenFieldMappingsAreOmittedWithNonSerializedClass() throws Exception {
+ createEmployeeTable();
+
+ cache = createCacheAndCreateJdbcMappingWithNonSerializedClass(
+ "NoFieldMappingsWithNonSerializedClass");
+ JdbcConnectorService service = cache.getService(JdbcConnectorService.class);
+
+ RegionMapping mapping = service.getMappingForRegion(REGION_NAME);
+ assertThat(mapping.getDataSourceName()).isEqualTo(DATA_SOURCE_NAME);
+ assertThat(mapping.getTableName()).isEqualTo(REGION_TABLE_NAME);
+ assertThat(mapping.getRegionName()).isEqualTo(REGION_NAME);
+ assertThat(mapping.getPdxName()).isEqualTo(NonSerializedEmployee.class.getName());
+ assertThat(mapping.getIds()).isEqualTo("id");
+ assertThat(mapping.getFieldMappings().size()).isEqualTo(3);
+ assertThat(mapping.getFieldMappings()).containsAll(getEmployeeTableFieldMappings());
+ }
+
+ @Test
+ public void mappingFailureWhenConnectWrongDataSource() {
+ Throwable throwable =
+ catchThrowable(() -> createCacheAndCreateJdbcMappingWithWrongDataSource("NoFieldMappings"));
+
+ assertThat(throwable).isInstanceOf(JdbcConnectorException.class)
+ .hasMessage(String.format("No datasource \"%s\" found when creating default field mapping",
+ DATA_SOURCE_NAME));
+ }
+
+ @Test
+ public void mappingFailureWhenTableNotExists() {
+ Throwable throwable = catchThrowable(() -> createCacheAndCreateJdbcMapping("NoFieldMappings"));
+
+ assertThat(throwable).isInstanceOf(JdbcConnectorException.class)
+ .hasMessage(String.format("No table was found that matches \"%s\"", REGION_TABLE_NAME));
+ }
+
+ @Test
+ public void mappingFailureWhenPdxNotExists() throws Exception {
+ createEmployeeTable();
+
+ Throwable throwable =
+ catchThrowable(() -> createCacheAndCreateJdbcMappingWithWrongPdxName("NoPdxName"));
Review comment:
This is a bug. I believe the string in the quote should be `WrongPdxName` instead of `NoPdxName`. There is no such file `CacheXmlJdbcMappingIntegrationTest.NoPdxName.cache.xml`. So the actual error is something like `FileNotFoundException` returned by `getXmlFileForTest()`. And the assertion at line 241 and 242 doesn't really catch the bug.
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+
+import java.net.URL;
+import java.sql.Connection;
+import java.sql.JDBCType;
+import java.sql.Statement;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.connectors.jdbc.internal.JdbcConnectorService;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.connectors.jdbc.test.junit.rules.DatabaseConnectionRule;
+import org.apache.geode.connectors.jdbc.test.junit.rules.MySqlConnectionRule;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.jndi.JNDIInvoker;
+import org.apache.geode.pdx.FieldType;
+import org.apache.geode.pdx.internal.AutoSerializableManager;
+
+public class CacheXmlJdbcMappingIntegrationTest {
+
+ private static final URL COMPOSE_RESOURCE_PATH =
+ CacheXmlJdbcMappingIntegrationTest.class.getResource("mysql.yml");
+ protected static final String DATA_SOURCE_NAME = "TestDataSource";
+ protected static final String DB_NAME = "test";
+ protected static final String REGION_TABLE_NAME = "employees";
+ protected static final String REGION_NAME = "Region1";
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ private Connection connection;
+ private Statement statement;
+ private InternalCache cache;
+
+ @ClassRule
+ public static DatabaseConnectionRule dbRule = new MySqlConnectionRule.Builder()
+ .file(COMPOSE_RESOURCE_PATH.getPath()).serviceName("db").port(3306).database(DB_NAME).build();
+
+ @Before
+ public void setUp() throws Exception {
+ System.setProperty(AutoSerializableManager.NO_HARDCODED_EXCLUDES_PARAM, "true");
+ connection = dbRule.getConnection();
+ statement = connection.createStatement();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ JNDIInvoker.unMapDatasource(DATA_SOURCE_NAME);
+
+ if (cache != null) {
+ cache.close();
+ }
+
+ if (statement == null) {
+ statement = connection.createStatement();
+ }
+ statement.execute("Drop table IF EXISTS " + REGION_TABLE_NAME);
+ statement.close();
+
+ if (connection != null) {
+ connection.close();
+ }
+ }
+
+ private InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
Review comment:
This `throws Exception` can be removed. The same for line 93, 109 and 119.
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+
+import java.net.URL;
+import java.sql.Connection;
+import java.sql.JDBCType;
+import java.sql.Statement;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.connectors.jdbc.internal.JdbcConnectorService;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.connectors.jdbc.test.junit.rules.DatabaseConnectionRule;
+import org.apache.geode.connectors.jdbc.test.junit.rules.MySqlConnectionRule;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.jndi.JNDIInvoker;
+import org.apache.geode.pdx.FieldType;
+import org.apache.geode.pdx.internal.AutoSerializableManager;
+
+public class CacheXmlJdbcMappingIntegrationTest {
+
+ private static final URL COMPOSE_RESOURCE_PATH =
+ CacheXmlJdbcMappingIntegrationTest.class.getResource("mysql.yml");
+ protected static final String DATA_SOURCE_NAME = "TestDataSource";
+ protected static final String DB_NAME = "test";
+ protected static final String REGION_TABLE_NAME = "employees";
+ protected static final String REGION_NAME = "Region1";
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ private Connection connection;
+ private Statement statement;
+ private InternalCache cache;
+
+ @ClassRule
+ public static DatabaseConnectionRule dbRule = new MySqlConnectionRule.Builder()
+ .file(COMPOSE_RESOURCE_PATH.getPath()).serviceName("db").port(3306).database(DB_NAME).build();
+
+ @Before
+ public void setUp() throws Exception {
+ System.setProperty(AutoSerializableManager.NO_HARDCODED_EXCLUDES_PARAM, "true");
+ connection = dbRule.getConnection();
+ statement = connection.createStatement();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ JNDIInvoker.unMapDatasource(DATA_SOURCE_NAME);
+
+ if (cache != null) {
+ cache.close();
+ }
+
+ if (statement == null) {
+ statement = connection.createStatement();
+ }
+ statement.execute("Drop table IF EXISTS " + REGION_TABLE_NAME);
+ statement.close();
+
+ if (connection != null) {
+ connection.close();
+ }
+ }
+
+ private InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
+ System.setProperty("TestDataSourceUrl", url);
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithNonSerializedClass(
+ String cacheXmlTestName) throws Exception {
+ return createCacheAndCreateJdbcMapping(cacheXmlTestName);
Review comment:
I would recommend inline this one-line implementation for readability of code.
##########
File path: geode-connectors/src/acceptanceTest/java/org/apache/geode/connectors/jdbc/CacheXmlJdbcMappingIntegrationTest.java
##########
@@ -0,0 +1,284 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.connectors.jdbc;
+
+import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
+
+import java.net.URL;
+import java.sql.Connection;
+import java.sql.JDBCType;
+import java.sql.Statement;
+import java.util.Arrays;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+
+import org.apache.geode.cache.CacheFactory;
+import org.apache.geode.connectors.jdbc.internal.JdbcConnectorService;
+import org.apache.geode.connectors.jdbc.internal.configuration.FieldMapping;
+import org.apache.geode.connectors.jdbc.internal.configuration.RegionMapping;
+import org.apache.geode.connectors.jdbc.test.junit.rules.DatabaseConnectionRule;
+import org.apache.geode.connectors.jdbc.test.junit.rules.MySqlConnectionRule;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.jndi.JNDIInvoker;
+import org.apache.geode.pdx.FieldType;
+import org.apache.geode.pdx.internal.AutoSerializableManager;
+
+public class CacheXmlJdbcMappingIntegrationTest {
+
+ private static final URL COMPOSE_RESOURCE_PATH =
+ CacheXmlJdbcMappingIntegrationTest.class.getResource("mysql.yml");
+ protected static final String DATA_SOURCE_NAME = "TestDataSource";
+ protected static final String DB_NAME = "test";
+ protected static final String REGION_TABLE_NAME = "employees";
+ protected static final String REGION_NAME = "Region1";
+
+ @Rule
+ public RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties();
+
+ private Connection connection;
+ private Statement statement;
+ private InternalCache cache;
+
+ @ClassRule
+ public static DatabaseConnectionRule dbRule = new MySqlConnectionRule.Builder()
+ .file(COMPOSE_RESOURCE_PATH.getPath()).serviceName("db").port(3306).database(DB_NAME).build();
+
+ @Before
+ public void setUp() throws Exception {
+ System.setProperty(AutoSerializableManager.NO_HARDCODED_EXCLUDES_PARAM, "true");
+ connection = dbRule.getConnection();
+ statement = connection.createStatement();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ JNDIInvoker.unMapDatasource(DATA_SOURCE_NAME);
+
+ if (cache != null) {
+ cache.close();
+ }
+
+ if (statement == null) {
+ statement = connection.createStatement();
+ }
+ statement.execute("Drop table IF EXISTS " + REGION_TABLE_NAME);
+ statement.close();
+
+ if (connection != null) {
+ connection.close();
+ }
+ }
+
+ private InternalCache createCacheAndCreateJdbcMapping(String cacheXmlTestName)
+ throws Exception {
+ String url = dbRule.getConnectionUrl().replaceAll("&", "&");
+ System.setProperty("TestDataSourceUrl", url);
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithNonSerializedClass(
+ String cacheXmlTestName) throws Exception {
+ return createCacheAndCreateJdbcMapping(cacheXmlTestName);
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithWrongDataSource(
+ String cacheXmlTestName) throws Exception {
+ System.setProperty("TestDataSourceUrl", "jdbc:mysql://localhost/test");
+ InternalCache cache =
+ (InternalCache) new CacheFactory().set("locators", "").set("mcast-port", "0")
+ .set("cache-xml-file", getXmlFileForTest(cacheXmlTestName))
+ .create();
+ return cache;
+ }
+
+ private InternalCache createCacheAndCreateJdbcMappingWithWrongPdxName(String cacheXmlTestName)
Review comment:
This is method implementation is exactly the same as `createCacheAndCreateJdbcMapping()`. The only difference is the method name. So how about just using `createCacheAndCreateJdbcMapping()`?
##########
File path: geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/xml/RegionMappingConfiguration.java
##########
@@ -62,4 +96,159 @@ private void createRegionMapping(JdbcConnectorService service,
throw new InternalGemFireException(e);
}
}
+
+ protected List<FieldMapping> createDefaultFieldMapping(RegionMapping regionMapping,
+ PdxType pdxType) {
+ DataSource dataSource = getDataSource(regionMapping.getDataSourceName());
+ if (dataSource == null) {
+ throw new JdbcConnectorException("No datasource \"" + regionMapping.getDataSourceName()
+ + "\" found when creating default field mapping");
+ }
+ TableMetaDataManager manager = getTableMetaDataManager();
+ try (Connection connection = dataSource.getConnection()) {
+ TableMetaDataView tableMetaData = manager.getTableMetaDataView(connection, regionMapping);
+ return createDefaultFieldMapping(regionMapping, pdxType, tableMetaData);
+ } catch (SQLException e) {
+ throw JdbcConnectorException.createException(e);
+ }
+ }
+
+ private List<FieldMapping> createDefaultFieldMapping(RegionMapping regionMapping,
Review comment:
The `regionMapping` argument is not used, and can be removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on pull request #5637: GEODE-8626: Omitting field-mapping tag of cache.xml when using Simple JDBC Connector
Posted by GitBox <gi...@apache.org>.
agingade commented on pull request #5637:
URL: https://github.com/apache/geode/pull/5637#issuecomment-854120507
@masaki-yamakawa
My worry is abandoning the xml support in near future. If you insist to go with this approach, its fine with me.
One change that could help code maintainability is moving your changes (which is duplicate of the code/logic from CreateMappingPreconditionCheckFunction) to RegionMapping as helper methods and then accessing it from both place.
E.g.:
if (mapping.getFieldMappings().isEmpty() && mapping.getPdxName != null) {
mapping.createMappingUsingPdx();
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org