GEODE-6511 Change data source to use connection pool by default

geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/DescribeDataSourceCommandDUnitTest.java

@@ -61,7 +61,7 @@ public void before() throws Exception {
   @Test
   public void describeDataSourceForSimpleDataSource() {
     gfsh.executeAndAssertThat(
-        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword")
+        "create data-source --name=simple --pooled=false --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword")
         .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
 
     CommandResultAssert result = gfsh.executeAndAssertThat("describe data-source --name=simple");
@@ -87,7 +87,7 @@ private void teardownDatabase() {
   private void executeSql(String sql) {
     server.invoke(() -> {
       try {
-        DataSource ds = JNDIInvoker.getDataSource("simple");
+        DataSource ds = JNDIInvoker.getDataSource("pool");
         Connection conn = ds.getConnection();
         Statement sm = conn.createStatement();
         sm.execute(sql);
@@ -136,24 +136,24 @@ public void fromData(PdxReader reader) {
   @Test
   public void describeDataSourceUsedByRegionsListsTheRegionsInOutput() {
     gfsh.executeAndAssertThat(
-        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\"")
+        "create data-source --name=pool --url=\"jdbc:derby:memory:newDB;create=true\"")
         .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
     gfsh.executeAndAssertThat("create region --name=region1 --type=REPLICATE").statusIsSuccess();
     gfsh.executeAndAssertThat("create region --name=region2 --type=PARTITION").statusIsSuccess();
     setupDatabase();
     try {
       gfsh.executeAndAssertThat(
-          "create jdbc-mapping --region=region1 --data-source=simple --pdx-name="
+          "create jdbc-mapping --region=region1 --data-source=pool --pdx-name="
               + IdAndName.class.getName() + " --schema=mySchema");
       gfsh.executeAndAssertThat(
-          "create jdbc-mapping --region=region2 --data-source=simple --pdx-name="
+          "create jdbc-mapping --region=region2 --data-source=pool --pdx-name="
               + IdAndName.class.getName() + " --schema=mySchema");
 
-      CommandResultAssert result = gfsh.executeAndAssertThat("describe data-source --name=simple");
+      CommandResultAssert result = gfsh.executeAndAssertThat("describe data-source --name=pool");
 
       result.statusIsSuccess()
-          .tableHasRowWithValues("Property", "Value", "name", "simple")
-          .tableHasRowWithValues("Property", "Value", "pooled", "false")
+          .tableHasRowWithValues("Property", "Value", "name", "pool")
+          .tableHasRowWithValues("Property", "Value", "pooled", "true")
           .tableHasRowWithValues("Property", "Value", "url", "jdbc:derby:memory:newDB;create=true");
       InfoResultModel infoSection = result.getResultModel()
           .getInfoSection(DescribeDataSourceCommand.REGIONS_USING_DATA_SOURCE_SECTION);

geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/DestroyDataSourceCommandDUnitTest.java

@@ -72,7 +72,7 @@ public void testDestroyDataSource() {
         .invokeInEveryMember(
             () -> assertThat(JNDIInvoker.getBindingNamesRecursively(JNDIInvoker.getJNDIContext()))
                 .containsKey("java:datasource1").containsValue(
-                    "org.apache.geode.internal.datasource.GemFireBasicDataSource"),
+                    "com.zaxxer.hikari.HikariDataSource"),
             server1, server2);
 
     gfsh.executeAndAssertThat("destroy data-source --name=datasource1").statusIsSuccess()
@@ -120,6 +120,69 @@ public void testDestroyDataSource() {
     });
   }
 
+  @Test
+  public void testDestroySimpleDataSource() throws Exception {
+    // drop the default pooled data source
+    gfsh.executeAndAssertThat("destroy data-source --name=datasource1").statusIsSuccess();
+
+    // create a simple data source, i.e. non-pooled data source
+    gfsh.execute(
+        "create data-source --name=datasource2 --url=\"jdbc:derby:memory:newDB;create=true\" --pooled=false");
+
+    // assert that there is a datasource
+    VMProvider
+        .invokeInEveryMember(
+            () -> assertThat(JNDIInvoker.getBindingNamesRecursively(JNDIInvoker.getJNDIContext()))
+                .containsKey("java:datasource2").containsValue(
+                    "org.apache.geode.internal.datasource.GemFireBasicDataSource"),
+            server1, server2);
+
+
+    gfsh.executeAndAssertThat("destroy data-source --name=datasource2").statusIsSuccess()
+        .tableHasColumnOnlyWithValues("Member", "server-1", "server-2")
+        .tableHasColumnOnlyWithValues("Message",
+            "Data source \"datasource2\" destroyed on \"server-1\"",
+            "Data source \"datasource2\" destroyed on \"server-2\"");
+
+    // verify cluster config is updated
+    locator.invoke(() -> {
+      InternalLocator internalLocator = ClusterStartupRule.getLocator();
+      AssertionsForClassTypes.assertThat(internalLocator).isNotNull();
+      InternalConfigurationPersistenceService ccService =
+          internalLocator.getConfigurationPersistenceService();
+      Configuration configuration = ccService.getConfiguration("cluster");
+      Document document = XmlUtils.createDocumentFromXml(configuration.getCacheXmlContent());
+      NodeList jndiBindings = document.getElementsByTagName("jndi-binding");
+
+      AssertionsForClassTypes.assertThat(jndiBindings.getLength()).isEqualTo(0);
+
+      boolean found = false;
+      for (int i = 0; i < jndiBindings.getLength(); i++) {
+        Element eachBinding = (Element) jndiBindings.item(i);
+        if (eachBinding.getAttribute("jndi-name").equals("datasource2")) {
+          found = true;
+          break;
+        }
+      }
+      AssertionsForClassTypes.assertThat(found).isFalse();
+    });
+
+    // verify datasource does not exists
+    VMProvider.invokeInEveryMember(
+        () -> AssertionsForClassTypes.assertThat(JNDIInvoker.getNoOfAvailableDataSources())
+            .isEqualTo(0),
+        server1, server2);
+
+    // bounce server1 and assert that there is still no datasource received from cluster config
+    server1.stop(false);
+    server1 = cluster.startServerVM(1, locator.getPort());
+
+    // verify no datasource from cluster config
+    server1.invoke(() -> {
+      AssertionsForClassTypes.assertThat(JNDIInvoker.getNoOfAvailableDataSources()).isEqualTo(0);
+    });
+  }
+
   private void createTable() {
     executeSql("create table mySchema.myRegion (id varchar(10) primary key, name varchar(10))");
   }
@@ -196,4 +259,27 @@ public void destroyDataSourceFailsIfInUseByJdbcMapping() {
       dropTable();
     }
   }
+
+  @Test
+  public void destroySimpleDataSourceFailsIfInUseByJdbcMapping() throws Exception {
+    // create a simple data source, i.e. non-pooled data source
+    gfsh.execute(
+        "create data-source --name=datasource2 --url=\"jdbc:derby:memory:newDB;create=true\" --pooled=false");
+
+    gfsh.executeAndAssertThat("create region --name=myRegion --type=REPLICATE").statusIsSuccess();
+    createTable();
+    try {
+      gfsh.executeAndAssertThat(
+          "create jdbc-mapping --data-source=datasource2 --pdx-name=" + IdAndName.class.getName()
+              + " --region=myRegion --schema=mySchema")
+          .statusIsSuccess();
+
+      gfsh.executeAndAssertThat("destroy data-source --name=datasource2").statusIsError()
+          .containsOutput(
+              "Data source named \"datasource2\" is still being used by region \"myRegion\"."
+                  + " Use destroy jdbc-mapping --region=myRegion and then try again.");
+    } finally {
+      dropTable();
+    }
+  }
 }

geode-connectors/src/distributedTest/java/org/apache/geode/connectors/jdbc/internal/cli/ListDataSourceCommandDUnitTest.java

@@ -56,7 +56,7 @@ public void before() throws Exception {
   @Test
   public void listDataSourceForSimpleDataSource() {
     gfsh.executeAndAssertThat(
-        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword")
+        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword --pooled=false ")
         .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
 
     CommandResultAssert result = gfsh.executeAndAssertThat("list data-source");
@@ -128,7 +128,7 @@ public void fromData(PdxReader reader) {
   @Test
   public void listDataSourceUsedByRegionsHasCorrectOutput() {
     gfsh.executeAndAssertThat(
-        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\"")
+        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\" --pooled=false")
         .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
     setupDatabase();
     gfsh.executeAndAssertThat("create region --name=region1 --type=REPLICATE").statusIsSuccess();
@@ -162,10 +162,22 @@ public void listDataSourceForPooledDataSource() {
             "jdbc:derby:memory:newDB;create=true");
   }
 
+  @Test
+  public void listDataSourceForPooledDataSourceByDefault() {
+    gfsh.executeAndAssertThat(
+        "create data-source --name=pooledDataSource --url=\"jdbc:derby:memory:newDB;create=true\" --pooled-data-source-factory-class=org.apache.geode.internal.jta.CacheJTAPooledDataSourceFactory --pool-properties={'name':'prop1','value':'value1'},{'name':'pool.prop2','value':'value2'}")
+        .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
+
+    gfsh.executeAndAssertThat("list data-source").statusIsSuccess()
+        .tableHasRowWithValues("name", "pooled", "in use", "url", "pooledDataSource", "true",
+            "false",
+            "jdbc:derby:memory:newDB;create=true");
+  }
+
   @Test
   public void listDataSourceWithMultipleDataSourcesListsAll() {
     gfsh.executeAndAssertThat(
-        "create data-source --name=simple --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword")
+        "create data-source --name=simple --pooled=false --url=\"jdbc:derby:memory:newDB;create=true\" --username=joe --password=myPassword")
         .statusIsSuccess().tableHasColumnOnlyWithValues("Member", "server-1");
     gfsh.executeAndAssertThat(
         "create data-source --name=pooledDataSource --pooled --url=\"jdbc:derby:memory:newDB;create=true\" --pooled-data-source-factory-class=org.apache.geode.internal.jta.CacheJTAPooledDataSourceFactory --pool-properties={'name':'prop1','value':'value1'},{'name':'pool.prop2','value':'value2'}")

geode-connectors/src/main/java/org/apache/geode/connectors/jdbc/internal/cli/CreateDataSourceCommand.java

@@ -91,12 +91,17 @@ public ResultModel createDataSource(
       @CliOption(key = CliStrings.IFNOTEXISTS, help = IFNOTEXISTS__HELP,
           specifiedDefaultValue = "true", unspecifiedDefaultValue = "false") boolean ifNotExists,
       @CliOption(key = POOLED, help = POOLED__HELP,
-          specifiedDefaultValue = "true", unspecifiedDefaultValue = "false") boolean pooled,
+          specifiedDefaultValue = "true", unspecifiedDefaultValue = "true") boolean pooled,
       @CliOption(key = POOL_PROPERTIES, optionContext = "splittingRegex=,(?![^{]*\\})",
           help = POOL_PROPERTIES_HELP) PoolProperty[] poolProperties) {
 
     JndiBindingsType.JndiBinding configuration = new JndiBindingsType.JndiBinding();
-    configuration.setConnPooledDatasourceClass(pooledDataSourceFactoryClass);
+    if (pooled) {
+      if (pooledDataSourceFactoryClass == null) {
+        pooledDataSourceFactoryClass = DEFAULT_POOLED_DATA_SOURCE_FACTORY_CLASS;
+      }
+      configuration.setConnPooledDatasourceClass(pooledDataSourceFactoryClass);
+    }
     configuration.setConnectionUrl(url);
     configuration.setJndiName(name);
     configuration.setPassword(password);

geode-core/src/integrationTest/java/org/apache/geode/management/internal/cli/functions/DestroyJndiBindingFunctionTest.java

@@ -134,7 +134,7 @@ public void destroyDataSourceFailsWhenBindingHasInvalidType() throws Exception {
     when(context.getArguments()).thenReturn(new Object[] {"jndi1", true});
     when(context.getResultSender()).thenReturn(resultSender);
     DestroyJndiBindingFunction destroyFunctionSpy = spy(destroyJndiBindingFunction);
-    doReturn(true).when(destroyFunctionSpy).checkForInvalidDataSource(any());
+    doReturn(false).when(destroyFunctionSpy).isValidDataSource(any());
 
     destroyFunctionSpy.execute(context);
     verify(resultSender).lastResult(resultCaptor.capture());

geode-core/src/main/java/org/apache/geode/internal/jndi/JNDIInvoker.java

@@ -48,8 +48,7 @@
 import org.apache.geode.internal.datasource.ConfigProperty;
 import org.apache.geode.internal.datasource.DataSourceCreateException;
 import org.apache.geode.internal.datasource.DataSourceFactory;
-import org.apache.geode.internal.datasource.GemFireBasicDataSource;
-import org.apache.geode.internal.datasource.GemFireConnPooledDataSource;
+import org.apache.geode.internal.datasource.GemFireTransactionDataSource;
 import org.apache.geode.internal.jta.TransactionManagerImpl;
 import org.apache.geode.internal.jta.TransactionUtils;
 import org.apache.geode.internal.jta.UserTransactionImpl;
@@ -406,14 +405,15 @@ public static DataSource getDataSource(String name) {
     }
   }
 
-  public static boolean checkForInvalidDataSource(String name) {
+  public static boolean isValidDataSource(String name) {
     Object dataSource = dataSourceMap.get(name);
 
-    if (dataSource == null || dataSource instanceof GemFireBasicDataSource
-        || dataSource instanceof GemFireConnPooledDataSource) {
-      return false;
+    if (dataSource == null || (dataSource != null && dataSource instanceof DataSource
+        && !(dataSource instanceof GemFireTransactionDataSource))) {
+      return true;
     }
-    return true;
+
+    return false;
   }
 
   /**

geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DestroyJndiBindingFunction.java

@@ -32,7 +32,7 @@ public CliFunctionResult executeFunction(FunctionContext<Object[]> context) {
 
     if (destroyingDataSource) {
       typeName = "Data source";
-      if (checkForInvalidDataSource(jndiName)) {
+      if (!isValidDataSource(jndiName)) {
         return new CliFunctionResult(context.getMemberName(), CliFunctionResult.StatusState.ERROR,
             CliStrings.format(
                 "Data Source {0} has invalid type for destroy data-source, destroy jndi-binding command should be used.",
@@ -53,7 +53,7 @@ public CliFunctionResult executeFunction(FunctionContext<Object[]> context) {
     }
   }
 
-  boolean checkForInvalidDataSource(String jndiName) {
-    return JNDIInvoker.checkForInvalidDataSource(jndiName);
+  boolean isValidDataSource(String jndiName) {
+    return JNDIInvoker.isValidDataSource(jndiName);
   }
 }