geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mcmellaw...@apache.org
Subject [geode] branch release/1.10.0 updated: Revert "GEODE-6973: Use cachelistener to synchronize typeToId with IdToType r… (#3853)"
Date Tue, 06 Aug 2019 21:18:22 GMT
This is an automated email from the ASF dual-hosted git repository.

mcmellawatt pushed a commit to branch release/1.10.0
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/release/1.10.0 by this push:
     new 1067733  Revert "GEODE-6973: Use cachelistener to synchronize typeToId with IdToType
r… (#3853)"
1067733 is described below

commit 10677330666d6bbc45ac535268db026d1351872e
Author: Naburun Nag <nag@cs.wisc.edu>
AuthorDate: Tue Aug 6 14:17:48 2019 -0700

    Revert "GEODE-6973: Use cachelistener to synchronize typeToId with IdToType r… (#3853)"
    
    This reverts commit 181e3a4a465aa9f5e06f39cd1285c94f9bc78600.
---
 .../geode/pdx/PdxTypeGenerationDUnitTest.java      | 320 ---------------------
 .../org/apache/geode/pdx/jsonStrings/testJSON.txt  |  43 ---
 .../apache/geode/pdx/JSONFormatterJUnitTest.java   | 247 +---------------
 .../PeerTypeRegistrationIntegrationTest.java       |  92 ------
 .../geode/internal/pdx/jsonStrings/testJSON.txt    |  43 ---
 .../geode/pdx/internal/PeerTypeRegistration.java   |  47 ++-
 6 files changed, 30 insertions(+), 762 deletions(-)

diff --git a/geode-core/src/distributedTest/java/org/apache/geode/pdx/PdxTypeGenerationDUnitTest.java
b/geode-core/src/distributedTest/java/org/apache/geode/pdx/PdxTypeGenerationDUnitTest.java
deleted file mode 100644
index d0fc78a..0000000
--- a/geode-core/src/distributedTest/java/org/apache/geode/pdx/PdxTypeGenerationDUnitTest.java
+++ /dev/null
@@ -1,320 +0,0 @@
-/*
- * 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.pdx;
-
-import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
-import static org.assertj.core.api.Assertions.assertThat;
-
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Properties;
-
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import util.TestException;
-
-import org.apache.geode.cache.DataPolicy;
-import org.apache.geode.cache.Region;
-import org.apache.geode.cache.client.ClientCache;
-import org.apache.geode.cache.client.ClientRegionShortcut;
-import org.apache.geode.internal.cache.InternalCache;
-import org.apache.geode.pdx.internal.PeerTypeRegistration;
-import org.apache.geode.test.dunit.AsyncInvocation;
-import org.apache.geode.test.dunit.rules.ClientVM;
-import org.apache.geode.test.dunit.rules.ClusterStartupRule;
-import org.apache.geode.test.dunit.rules.MemberVM;
-
-public class PdxTypeGenerationDUnitTest {
-
-  private static final float TARGET_CONSISTENCY_FACTOR = 2f;
-
-  private static final String FIELD_NAME_TO_REPLACE = "maxUrlLength";
-
-  private MemberVM locator, server1, server2;
-
-  @Rule
-  public ClusterStartupRule cluster = new ClusterStartupRule();
-
-  @Before
-  public void before() {
-    Properties props = new Properties();
-    props.setProperty("log-level", "WARN");
-
-    locator = cluster.startLocatorVM(0, props);
-
-    int locatorPort1 = locator.getPort();
-    server1 = cluster.startServerVM(1,
-        x -> x.withProperties(props).withConnectionToLocator(locatorPort1));
-
-    int locatorPort2 = locator.getPort();
-    server2 = cluster.startServerVM(2,
-        x -> x.withProperties(props).withConnectionToLocator(locatorPort2));
-  }
-
-  @Test
-  public void testSortJSONFieldNamesDoesNotImpactPerformance() {
-    final String fileName = "/org/apache/geode/pdx/jsonStrings/testJSON.txt";
-    String jsonString = loadJSONFileAsString(fileName);
-
-    server1.invoke(() -> {
-      int repeats = 1000;
-      long totalUnsortedTime = 0;
-      long totalSortedTime = 0;
-
-      // The first JSON document used to create a PdxInstance takes significantly longer
than
-      // successive ones, so a dummy document is used to trigger this first, longer creation
process
-      JSONFormatter.fromJSON("{\"testFieldName\": \"test\"}");
-      for (int i = 0; i < repeats; ++i) {
-        System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY, "false");
-
-        String fieldOne = "counter" + 2 * i;
-
-        long startTime = System.currentTimeMillis();
-        JSONFormatter.fromJSON(jsonString.replace(FIELD_NAME_TO_REPLACE, fieldOne));
-        totalUnsortedTime += (System.currentTimeMillis() - startTime);
-
-        System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY, "true");
-
-        String fieldTwo = "counter" + 2 * i + 1;
-
-        startTime = System.currentTimeMillis();
-        JSONFormatter.fromJSON(jsonString.replace(FIELD_NAME_TO_REPLACE, fieldTwo));
-        totalSortedTime += (System.currentTimeMillis() - startTime);
-      }
-      float consistencyFactor = (float) totalUnsortedTime / (float) totalSortedTime;
-      assertThat(consistencyFactor).withFailMessage(
-          "Expected unsorted JSON to take no longer than %f times as long as sorted JSON.
Was actually %f times.",
-          TARGET_CONSISTENCY_FACTOR, consistencyFactor)
-          .isLessThan(TARGET_CONSISTENCY_FACTOR);
-
-      assertThat(1 / consistencyFactor).withFailMessage(
-          "Expected sorted JSON to take no longer than %f times as long as unsorted JSON.
Was actually %f times.",
-          TARGET_CONSISTENCY_FACTOR, 1 / consistencyFactor)
-          .isLessThan(TARGET_CONSISTENCY_FACTOR);
-    });
-  }
-
-  @Test
-  public void testLocalPdxTypeMapRecoveredAfterServerRestart() {
-    final String fileName = "/org/apache/geode/pdx/jsonStrings/testJSON.txt";
-    String jsonString = loadJSONFileAsString(fileName);
-    final int addedTypes = 10;
-
-    server1.invoke(() -> {
-      InternalCache cache = ClusterStartupRule.getCache();
-      assertThat(cache).isNotNull();
-
-      // Creating a PdxType from the unmodified JSON document so that the same document can
be used
-      // later to confirm that the PdxType associated with it is still registered
-      JSONFormatter.fromJSON(jsonString);
-
-      // Creating more PdxTypes to allow us to confirm that they are all recovered when we
attempt
-      // to create a PdxType after restarting the server
-      for (int i = 0; i < addedTypes; ++i) {
-        String replacementField = "counter" + i;
-        String modifiedJSON = jsonString.replace(FIELD_NAME_TO_REPLACE, replacementField);
-        JSONFormatter.fromJSON(modifiedJSON);
-      }
-      PeerTypeRegistration registration =
-          (PeerTypeRegistration) (cache.getPdxRegistry().getTypeRegistration());
-
-      assertThat(registration.getLocalSize()).isEqualTo(addedTypes + 1);
-      assertThat(registration.getTypeToIdSize()).isEqualTo(addedTypes + 1);
-    });
-
-    server1.stop(false);
-    Properties props = new Properties();
-    props.setProperty("log-level", "WARN");
-    int locatorPort1 = locator.getPort();
-    server1 = cluster.startServerVM(1,
-        x -> x.withProperties(props).withConnectionToLocator(locatorPort1));
-
-    server1.invoke(() -> {
-      InternalCache cache = ClusterStartupRule.getCache();
-      assertThat(cache).isNotNull();
-
-      PeerTypeRegistration registration =
-          (PeerTypeRegistration) (cache.getPdxRegistry().getTypeRegistration());
-
-      assertThat(registration.getLocalSize()).isEqualTo(addedTypes + 1);
-      assertThat(registration.getTypeToIdSize()).isEqualTo(0);
-
-      // Attempting to create a PdxType that already exists in the /PdxTypes region in order
to
-      // trigger rebuilding of the TypeToId map in PeerTypeRegistration
-      JSONFormatter.fromJSON(jsonString);
-
-      assertThat(registration.getLocalSize()).isEqualTo(addedTypes + 1);
-      assertThat(registration.getTypeToIdSize()).isEqualTo(addedTypes + 1);
-    });
-  }
-
-  @Test
-  public void testNoConflictsWhenGeneratingPdxTypesFromJSONOnMultipleServers() {
-    int repeats = 1000;
-    AsyncInvocation invocation1 = null;
-    AsyncInvocation invocation2 = null;
-
-    invocation1 = server1.invokeAsync(() -> {
-      for (int i = 0; i < repeats; ++i) {
-        JSONFormatter.fromJSON("{\"fieldName" + i + "\": \"value\"}");
-      }
-    });
-    invocation2 = server2.invokeAsync(() -> {
-      for (int i = 0; i < repeats; ++i) {
-        JSONFormatter.fromJSON("{\"fieldName" + i + "\": \"value\"}");
-      }
-    });
-
-    try {
-      invocation1.await();
-      invocation2.await();
-    } catch (Exception ex) {
-      throw new TestException("Exception while awaiting async invocation: " + ex);
-    }
-
-    server1.invoke(() -> {
-      InternalCache cache = ClusterStartupRule.getCache();
-      assertThat(cache).isNotNull();
-      int numberOfTypesInRegion = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).size();
-      int numberOfTypesInLocalMap =
-          ((PeerTypeRegistration) cache.getPdxRegistry().getTypeRegistration()).getTypeToIdSize();
-
-      assertThat(numberOfTypesInRegion)
-          .withFailMessage("Expected number of PdxTypes in region to be %s but was %s",
-              repeats, numberOfTypesInRegion)
-          .isEqualTo(repeats);
-
-      assertThat(numberOfTypesInLocalMap)
-          .withFailMessage("Expected number of PdxTypes in local map to be %s but was %s",
-              repeats, numberOfTypesInLocalMap)
-          .isEqualTo(repeats);
-    });
-
-    server2.invoke(() -> {
-      InternalCache cache = ClusterStartupRule.getCache();
-      assertThat(cache).isNotNull();
-      int numberOfTypesInRegion = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).size();
-      int numberOfTypesInLocalMap =
-          ((PeerTypeRegistration) cache.getPdxRegistry().getTypeRegistration()).getTypeToIdSize();
-
-      assertThat(numberOfTypesInRegion)
-          .withFailMessage("Expected number of PdxTypes in region to be %s but was %s",
-              repeats, numberOfTypesInRegion)
-          .isEqualTo(repeats);
-
-      assertThat(numberOfTypesInLocalMap)
-          .withFailMessage("Expected number of PdxTypes in local map to be %s but was %s",
-              repeats, numberOfTypesInLocalMap)
-          .isEqualTo(repeats);
-    });
-  }
-
-  @Test
-  public void testPdxTypesGeneratedFromJSONOnClientAreEnteredIntoTypeRegistry() throws Exception
{
-    server1.invoke(() -> {
-      ClusterStartupRule.getCache().createRegionFactory().setDataPolicy(
-          DataPolicy.REPLICATE).create("Test");
-    });
-    server2.invoke(() -> {
-      ClusterStartupRule.getCache().createRegionFactory().setDataPolicy(
-          DataPolicy.REPLICATE).create("Test");
-    });
-    int port = locator.getPort();
-    int serverPort = server1.getPort();
-
-    Properties props = new Properties();
-    props.setProperty("log-level", "WARN");
-    ClientVM client = cluster.startClientVM(3,
-        cf -> cf.withLocatorConnection(port).withPoolSubscription(true).withProperties(props));
-
-    int entries = 10;
-    client.invoke(() -> {
-      ClientCache cache = ClusterStartupRule.getClientCache();
-      Region region =
-          cache.createClientRegionFactory(ClientRegionShortcut.CACHING_PROXY).create("Test");
-      String field;
-      String jsonString;
-
-      for (int i = 0; i < entries; ++i) {
-        field = "\"counter" + i + "\": " + i;
-
-        jsonString = "{" + field + "}";
-
-        JSONFormatter.fromJSON(jsonString);
-      }
-    });
-
-    server1.invoke(() -> {
-      InternalCache cache = ClusterStartupRule.getCache();
-      assertThat(cache).isNotNull();
-      int numberOfTypesInRegion = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).size();
-      int numberOfTypesInLocalMap =
-          ((PeerTypeRegistration) cache.getPdxRegistry().getTypeRegistration()).getTypeToIdSize();
-
-      assertThat(numberOfTypesInRegion)
-          .withFailMessage("Expected number of PdxTypes in region to be %s but was %s",
-              entries, numberOfTypesInRegion)
-          .isEqualTo(entries);
-
-      assertThat(numberOfTypesInLocalMap)
-          .withFailMessage("Expected number of PdxTypes in local map to be %s but was %s",
-              entries, numberOfTypesInLocalMap)
-          .isEqualTo(entries);
-    });
-  }
-
-  private String loadJSONFileAsString(String fileName) {
-    Path filePath = loadTestResourcePath(fileName);
-    String jsonString;
-    try {
-      jsonString = new String(Files.readAllBytes(filePath));
-    } catch (IOException ex) {
-      throw new TestException(ex.getMessage());
-    }
-    return jsonString;
-  }
-
-  private List<String> loadJSONFileAsList(String fileName) {
-    Path filePath = loadTestResourcePath(fileName);
-    List<String> jsonLines;
-    try {
-      jsonLines = new ArrayList<>(Files.readAllLines(filePath));
-    } catch (IOException ex) {
-      throw new TestException(ex.getMessage());
-    }
-    return jsonLines;
-  }
-
-  private static String buildJSONString(List<String> jsonLines) {
-    StringBuilder jsonString = new StringBuilder();
-    for (int i = 0; i < jsonLines.size(); ++i) {
-      String line = jsonLines.get(i);
-      jsonString.append(line + "\n");
-    }
-    return jsonString.toString();
-  }
-
-  private Path loadTestResourcePath(String fileName) {
-    Path filePath = createTempFileFromResource(getClass(), fileName).toPath();
-    assertThat(filePath).isNotNull();
-
-    return filePath;
-  }
-
-}
diff --git a/geode-core/src/distributedTest/resources/org/apache/geode/pdx/jsonStrings/testJSON.txt
b/geode-core/src/distributedTest/resources/org/apache/geode/pdx/jsonStrings/testJSON.txt
deleted file mode 100644
index 735fa70..0000000
--- a/geode-core/src/distributedTest/resources/org/apache/geode/pdx/jsonStrings/testJSON.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-{
-    "configGlossary:installationAt": "Philadelphia, PA",
-    "configGlossary:adminEmail": "ksm@pobox.com",
-    "configGlossary:poweredBy": "Cofax",
-    "configGlossary:poweredByIcon": "/images/cofax.gif",
-    "configGlossary:staticPath": "/content/static",
-    "templateProcessorClass": "org.cofax.WysiwygTemplate",
-    "templateLoaderClass": "org.cofax.FilesTemplateLoader",
-    "templatePath": "templates",
-    "templateOverridePath": "",
-    "defaultListTemplate": "listTemplate.htm",
-    "defaultFileTemplate": "articleTemplate.htm",
-    "useJSP": false,
-    "jspListTemplate": "listTemplate.jsp",
-    "jspFileTemplate": "articleTemplate.jsp",
-    "cachePackageTagsTrack": 200,
-    "cachePackageTagsStore": 200,
-    "cachePackageTagsRefresh": 60,
-    "cacheTemplatesTrack": 100,
-    "cacheTemplatesStore": 50,
-    "cacheTemplatesRefresh": 15,
-    "cachePagesTrack": 200,
-    "cachePagesStore": 100,
-    "cachePagesRefresh": 10,
-    "cachePagesDirtyRead": 10,
-    "searchEngineListTemplate": "forSearchEnginesList.htm",
-    "searchEngineFileTemplate": "forSearchEngines.htm",
-    "searchEngineRobotsDb": "WEB-INF/robots.db",
-    "useDataStore": true,
-    "dataStoreClass": "org.cofax.SqlDataStore",
-    "redirectionClass": "org.cofax.SqlRedirection",
-    "dataStoreName": "cofax",
-    "dataStoreDriver": "com.microsoft.jdbc.sqlserver.SQLServerDriver",
-    "dataStoreUrl": "jdbc:microsoft:sqlserver://LOCALHOST:1433;DatabaseName=goon",
-    "dataStoreUser": "sa",
-    "dataStorePassword": "dataStoreTestQuery",
-    "dataStoreTestQuery": "SET NOCOUNT ON;select test='test';",
-    "dataStoreLogFile": "/usr/local/tomcat/logs/datastore.log",
-    "dataStoreInitConns": 10,
-    "dataStoreMaxConns": 100,
-    "dataStoreConnUsageLimit": 100,
-    "dataStoreLogLevel": "debug",
-    "maxUrlLength": 500}
diff --git a/geode-core/src/integrationTest/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java
index f8096f9..0995fb8 100755
--- a/geode-core/src/integrationTest/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java
+++ b/geode-core/src/integrationTest/java/org/apache/geode/pdx/JSONFormatterJUnitTest.java
@@ -15,20 +15,11 @@
 package org.apache.geode.pdx;
 
 import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
-import static org.apache.geode.test.util.ResourceUtils.createTempFileFromResource;
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
-import java.io.File;
-import java.io.FileNotFoundException;
 import java.text.SimpleDateFormat;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
-import java.util.Scanner;
 
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -36,32 +27,26 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
-import util.TestException;
 
 import org.apache.geode.cache.Cache;
 import org.apache.geode.cache.CacheFactory;
 import org.apache.geode.cache.DataPolicy;
 import org.apache.geode.cache.Region;
 import org.apache.geode.pdx.internal.PdxInstanceImpl;
-import org.apache.geode.pdx.internal.PdxType;
 import org.apache.geode.pdx.internal.PeerTypeRegistration;
 import org.apache.geode.test.junit.categories.SerializationTest;
 
 @Category({SerializationTest.class})
 public class JSONFormatterJUnitTest {
   public static final String REGION_NAME = "primitiveKVStore";
-
   private Cache cache;
-
   private Region<Object, Object> region;
 
   @Before
   public void setUp() throws Exception {
-    this.cache = new CacheFactory().set(MCAST_PORT, "0")
-        .set("log-level", "WARN").setPdxReadSerialized(true).create();
+    this.cache = new CacheFactory().set(MCAST_PORT, "0").setPdxReadSerialized(true).create();
 
-    region = cache.createRegionFactory().setDataPolicy(DataPolicy.PARTITION)
-        .create(REGION_NAME);
+    region = cache.createRegionFactory().setDataPolicy(DataPolicy.PARTITION).create(REGION_NAME);
 
   }
 
@@ -104,8 +89,7 @@ public class JSONFormatterJUnitTest {
     // 2. Get the JSON string from actualTestObject using jackson ObjectMapper.
     ObjectMapper objectMapper = new ObjectMapper();
     objectMapper.setDateFormat(new SimpleDateFormat("MM/dd/yyyy"));
-    objectMapper
-        .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
+    objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
 
     String json = objectMapper.writeValueAsString(expectedTestObject);
 
@@ -127,16 +111,14 @@ public class JSONFormatterJUnitTest {
   }
 
   @Test
-  public void verifyJsonToPdxInstanceConversionWithJSONFormatter()
-      throws Exception {
+  public void verifyJsonToPdxInstanceConversionWithJSONFormatter() throws Exception {
     TestObjectForJSONFormatter expectedTestObject = new TestObjectForJSONFormatter();
     expectedTestObject.defaultInitialization();
 
     // 1.gets pdxInstance using R.put() and R.get()
     region.put("501", expectedTestObject);
     Object receivedObject = region.get("501");
-    assertEquals("receivedObject is expected to be of type PdxInstance",
-        PdxInstanceImpl.class,
+    assertEquals("receivedObject is expected to be of type PdxInstance", PdxInstanceImpl.class,
         receivedObject.getClass());
 
     PdxInstance expectedPI = (PdxInstance) receivedObject;
@@ -158,8 +140,7 @@ public class JSONFormatterJUnitTest {
     assertEquals("receivedObject is expected to be of type PdxInstance",
         TestObjectForJSONFormatter.class, actualTestObject.getClass());
 
-    assertEquals("actualTestObject and expectedTestObject should be equal",
-        expectedTestObject,
+    assertEquals("actualTestObject and expectedTestObject should be equal", expectedTestObject,
         actualTestObject);
   }
 
@@ -172,8 +153,7 @@ public class JSONFormatterJUnitTest {
     int pdxTypes = 0;
 
     if (cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH) != null) {
-      pdxTypes = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).keySet()
-          .size();
+      pdxTypes = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).keySet().size();
     }
 
     String js = "{name:\"ValueExist\", age:14}";
@@ -217,8 +197,7 @@ public class JSONFormatterJUnitTest {
       int pdxTypes = 0;
 
       if (cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH) != null) {
-        pdxTypes = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH)
-            .keySet().size();
+        pdxTypes = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH).keySet().size();
       }
 
       String js2 = "{c:\"c' go\", bb:23, b:\"b\", age:14 }";
@@ -231,214 +210,4 @@ public class JSONFormatterJUnitTest {
       System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY, "false");
     }
   }
-
-  @Test
-  public void testReorderJSONFieldsDoesNotCauseManyTypeIdConflicts() {
-    final int entries = 10000;
-    final float toleratedCollisonFraction = 0.01f;
-
-    List<String> jsonLines = loadJSONDocument();
-
-    for (int i = 0; i < entries; ++i) {
-      randomizeFieldsAndCreatePdx(jsonLines);
-    }
-
-    int collisons = calculateNumberOfCollisions();
-
-    float collisonRate = (float) collisons / (float) entries;
-
-    assertThat(collisonRate)
-        .withFailMessage("PdxTypeId collision rate too high. Expected %f but was %f.",
-            toleratedCollisonFraction, collisonRate)
-        .isLessThan(toleratedCollisonFraction);
-  }
-
-  @Test
-  public void testCounterInJSONFieldNameDoesNotCauseManyTypeIdConflicts() {
-    final int entries = 10000;
-    final float toleratedCollisonFraction = 0.01f;
-
-    List<String> jsonLines = loadJSONDocument();
-
-    String jsonString = "";
-    String counterField = "";
-    String fieldToReplace = jsonLines.get(1);
-    for (int i = 0; i < entries; ++i) {
-      jsonString = buildJSONString(jsonLines);
-      counterField = "\"counter" + i + "\": " + i + ",";
-      JSONFormatter.fromJSON(jsonString.replace(fieldToReplace, counterField));
-    }
-
-    int collisons = calculateNumberOfCollisions();
-
-    float collisonRate = (float) collisons / (float) entries;
-
-    assertThat(collisonRate)
-        .withFailMessage("PdxTypeId collision rate too high. Expected %f but was %f.",
-            toleratedCollisonFraction, collisonRate)
-        .isLessThan(toleratedCollisonFraction);
-  }
-
-  @Test
-  public void testSortingJSONFieldsPreventsTypeIDCollision() {
-    String initialState =
-        String.valueOf(
-            Boolean.getBoolean(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY));
-    try {
-      final int unsortedEntries = 100;
-      final int sortedEntries = 10000;
-      final int jsonFields = 40;
-
-      List<String> jsonLines = generateJSONWithCollidingFieldNames(jsonFields);
-
-      System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY, "false");
-
-      // Show that not sorting leads to 100% collision rate. There are 1 fewer collisions
than there
-      // are entries because the first entry cannot have a TypeId collision
-      for (int i = 0; i < unsortedEntries; ++i) {
-        randomizeFieldsAndCreatePdx(jsonLines);
-      }
-      int unsortedCollisions = calculateNumberOfCollisions();
-      assertThat(unsortedCollisions).withFailMessage(
-          "Unexpected number of PdxTypeId collisions. Expected %d but was actually %d.",
-          unsortedEntries - 1, unsortedCollisions).isEqualTo(unsortedEntries - 1);
-
-      System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY, "true");
-
-      // Show that sorting leads to no additional collisions beyond the single probable collision
-      // caused by the first sorted entry
-      for (int i = 0; i < sortedEntries; ++i) {
-        randomizeFieldsAndCreatePdx(jsonLines);
-      }
-      int sortedCollisions = calculateNumberOfCollisions();
-      assertThat(sortedCollisions)
-          .withFailMessage("Too many PdxTypeId collisions. Expected %d but was %d.",
-              unsortedCollisions + 1, sortedCollisions)
-          .isLessThanOrEqualTo(unsortedCollisions + 1);
-    } finally {
-      System.setProperty(JSONFormatter.SORT_JSON_FIELD_NAMES_PROPERTY,
-          initialState);
-    }
-  }
-
-  public List<String> loadJSONDocument() {
-    File source = loadTestResource(
-        "/org/apache/geode/internal/pdx/jsonStrings/testJSON.txt");
-    assertThat(source.exists());
-    return getJSONLines(source);
-  }
-
-  private File loadTestResource(String fileName) {
-    String filePath = createTempFileFromResource(getClass(), fileName)
-        .getAbsolutePath();
-    assertThat(filePath).isNotNull();
-
-    return new File(filePath);
-  }
-
-  private List<String> getJSONLines(File source) {
-    try {
-      Scanner scanner = new Scanner(source);
-      List<String> jsonLines = new ArrayList<>();
-      while (scanner.hasNext()) {
-        jsonLines.add(scanner.nextLine());
-      }
-      return jsonLines;
-    } catch (FileNotFoundException e) {
-      throw new TestException(e.getMessage());
-    }
-  }
-
-  private String buildJSONString(List<String> jsonLines) {
-    StringBuilder jsonString = new StringBuilder();
-    for (int i = 0; i < jsonLines.size(); ++i) {
-      String line = jsonLines.get(i);
-      jsonString.append(line + "\n");
-    }
-    return jsonString.toString();
-  }
-
-  private void randomizeFieldsAndCreatePdx(List<String> jsonLines) {
-    String jsonString = "";
-    // First and last lines of the JSON document cannot be reordered due to containing "{"
and "}"
-    Collections.shuffle(jsonLines.subList(1, jsonLines.size() - 1));
-    jsonString = buildJSONString(jsonLines);
-    JSONFormatter.fromJSON(jsonString);
-  }
-
-  private int calculateNumberOfCollisions() {
-    Collection pdxTypes = null;
-
-    if (cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH) != null) {
-      pdxTypes = cache.getRegion(PeerTypeRegistration.REGION_FULL_PATH)
-          .values();
-    }
-    assertThat(pdxTypes).isNotNull();
-
-    int collisions = 0;
-    for (Object object : pdxTypes) {
-      PdxType pdxType = (PdxType) object;
-
-      // Ideally this would be a method call to PeerTypeRegistration to avoid code duplication
-      int calculatedId =
-          pdxType.hashCode() & PeerTypeRegistration.PLACE_HOLDER_FOR_TYPE_ID;
-
-      if (pdxType.getTypeId() != calculatedId) {
-        collisions++;
-      }
-    }
-    return collisions;
-  }
-
-  private List<String> generateJSONWithCollidingFieldNames(int jsonFields) {
-    List<String> jsonLines = generateCollidingStrings(jsonFields);
-    assertThat(jsonLines).isNotNull();
-
-    // Format generated strings into JSON document
-    for (int i = 0; i < jsonLines.size(); ++i) {
-      String field = "\"" + jsonLines.get(i) + "\": " + i;
-      // Do not add a comma to the last field, but do add a closing }
-      if (i != jsonLines.size() - 1) {
-        field += ",";
-      } else {
-        field += "}";
-      }
-      jsonLines.set(i, field);
-    }
-    jsonLines.add(0, "{");
-    return jsonLines;
-  }
-
-  /**
-   * This method produces Strings with identical hashcode() values
-   */
-  private List<String> generateCollidingStrings(int numberOfStrings) {
-    final String[] baseStrings = {"Aa", "BB", "C#"};
-
-    int firstHash = baseStrings[0].hashCode();
-    for (String element : baseStrings) {
-      assertThat(element.hashCode() == firstHash);
-    }
-
-    // By definition, it requires at least two strings for there to be a collision
-    if (numberOfStrings < 2) {
-      return new ArrayList<>();
-    }
-
-    int elementsPerString =
-        (int) Math.ceil(Math.log(numberOfStrings) / Math.log(baseStrings.length));
-
-    List<String> result = Arrays.asList("", "", "");
-
-    for (int i = 0; i < elementsPerString; ++i) {
-      List<String> partialResult = new ArrayList();
-      for (String string : result) {
-        for (String element : baseStrings) {
-          partialResult.add(string + element);
-        }
-      }
-      result = partialResult;
-    }
-    return result.subList(0, numberOfStrings);
-  }
 }
diff --git a/geode-core/src/integrationTest/java/org/apache/geode/pdx/internal/PeerTypeRegistrationIntegrationTest.java
b/geode-core/src/integrationTest/java/org/apache/geode/pdx/internal/PeerTypeRegistrationIntegrationTest.java
deleted file mode 100644
index 680e723..0000000
--- a/geode-core/src/integrationTest/java/org/apache/geode/pdx/internal/PeerTypeRegistrationIntegrationTest.java
+++ /dev/null
@@ -1,92 +0,0 @@
-/*
- * 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.pdx.internal;
-
-import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
-import static org.assertj.core.api.Assertions.assertThat;
-
-import org.junit.After;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-
-import org.apache.geode.cache.Cache;
-import org.apache.geode.cache.CacheFactory;
-import org.apache.geode.internal.cache.InternalCache;
-import org.apache.geode.test.junit.categories.SerializationTest;
-
-@Category({SerializationTest.class})
-public class PeerTypeRegistrationIntegrationTest {
-  private Cache cache;
-  private PeerTypeRegistration registration;
-
-  @Before
-  public void setUp() {
-    cache = new CacheFactory().set(MCAST_PORT, "0")
-        .set("log-level", "WARN").setPdxReadSerialized(true).create();
-    registration = new PeerTypeRegistration((InternalCache) cache);
-    registration.initialize();
-  }
-
-  @After
-  public void tearDown() {
-    try {
-      if (!cache.isClosed()) {
-        cache.close();
-      }
-    } catch (Exception e) {
-      throw new AssertionError(e);
-    }
-  }
-
-  @Test
-  public void testDefineType() {
-
-    PdxType firstType = new PdxType();
-    firstType.setClassName("Mock.Test.Class.One");
-
-    PdxType secondType = new PdxType();
-    secondType.setClassName("Mock.Test.Class.Two");
-
-    assertThat(registration.getLocalSize()).isEqualTo(0);
-    assertThat(registration.getTypeToIdSize()).isEqualTo(0);
-
-    int firstTypeId1 = registration.defineType(firstType);
-
-    // Confirm the PdxType was added to the region and the local map
-    assertThat(registration.getLocalSize()).isEqualTo(1);
-    assertThat(registration.getTypeToIdSize()).isEqualTo(1);
-
-    firstType.setTypeId(firstTypeId1 - 1);
-    int firstTypeId2 = registration.defineType(firstType);
-
-    // Defining an existing type with a different TypeId returns the existing TypeId
-    assertThat(firstTypeId1).isEqualTo(firstTypeId2);
-    assertThat(registration.getType(firstTypeId2)).isEqualTo(firstType);
-
-    // Defining an existing type does not add a new type to the region or local map
-    assertThat(registration.getLocalSize()).isEqualTo(1);
-    assertThat(registration.getTypeToIdSize()).isEqualTo(1);
-
-    secondType.setTypeId(firstTypeId1);
-
-    int secondTypeId = registration.defineType(secondType);
-
-    // Defining a new type with an existing TypeId does not overwrite the existing type
-    assertThat(secondTypeId).isNotEqualTo(firstTypeId1);
-    assertThat(registration.getLocalSize()).isEqualTo(2);
-    assertThat(registration.getTypeToIdSize()).isEqualTo(2);
-  }
-}
diff --git a/geode-core/src/integrationTest/resources/org/apache/geode/internal/pdx/jsonStrings/testJSON.txt
b/geode-core/src/integrationTest/resources/org/apache/geode/internal/pdx/jsonStrings/testJSON.txt
deleted file mode 100644
index 735fa70..0000000
--- a/geode-core/src/integrationTest/resources/org/apache/geode/internal/pdx/jsonStrings/testJSON.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-{
-    "configGlossary:installationAt": "Philadelphia, PA",
-    "configGlossary:adminEmail": "ksm@pobox.com",
-    "configGlossary:poweredBy": "Cofax",
-    "configGlossary:poweredByIcon": "/images/cofax.gif",
-    "configGlossary:staticPath": "/content/static",
-    "templateProcessorClass": "org.cofax.WysiwygTemplate",
-    "templateLoaderClass": "org.cofax.FilesTemplateLoader",
-    "templatePath": "templates",
-    "templateOverridePath": "",
-    "defaultListTemplate": "listTemplate.htm",
-    "defaultFileTemplate": "articleTemplate.htm",
-    "useJSP": false,
-    "jspListTemplate": "listTemplate.jsp",
-    "jspFileTemplate": "articleTemplate.jsp",
-    "cachePackageTagsTrack": 200,
-    "cachePackageTagsStore": 200,
-    "cachePackageTagsRefresh": 60,
-    "cacheTemplatesTrack": 100,
-    "cacheTemplatesStore": 50,
-    "cacheTemplatesRefresh": 15,
-    "cachePagesTrack": 200,
-    "cachePagesStore": 100,
-    "cachePagesRefresh": 10,
-    "cachePagesDirtyRead": 10,
-    "searchEngineListTemplate": "forSearchEnginesList.htm",
-    "searchEngineFileTemplate": "forSearchEngines.htm",
-    "searchEngineRobotsDb": "WEB-INF/robots.db",
-    "useDataStore": true,
-    "dataStoreClass": "org.cofax.SqlDataStore",
-    "redirectionClass": "org.cofax.SqlRedirection",
-    "dataStoreName": "cofax",
-    "dataStoreDriver": "com.microsoft.jdbc.sqlserver.SQLServerDriver",
-    "dataStoreUrl": "jdbc:microsoft:sqlserver://LOCALHOST:1433;DatabaseName=goon",
-    "dataStoreUser": "sa",
-    "dataStorePassword": "dataStoreTestQuery",
-    "dataStoreTestQuery": "SET NOCOUNT ON;select test='test';",
-    "dataStoreLogFile": "/usr/local/tomcat/logs/datastore.log",
-    "dataStoreInitConns": 10,
-    "dataStoreMaxConns": 100,
-    "dataStoreConnUsageLimit": 100,
-    "dataStoreLogLevel": "debug",
-    "maxUrlLength": 500}
diff --git a/geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
b/geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
index f0c7d5d..0aae7bf 100644
--- a/geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
+++ b/geode-core/src/main/java/org/apache/geode/pdx/internal/PeerTypeRegistration.java
@@ -26,7 +26,6 @@ import org.apache.logging.log4j.Logger;
 
 import org.apache.geode.InternalGemFireError;
 import org.apache.geode.InternalGemFireException;
-import org.apache.geode.annotations.VisibleForTesting;
 import org.apache.geode.cache.AttributesFactory;
 import org.apache.geode.cache.CacheWriterException;
 import org.apache.geode.cache.DataPolicy;
@@ -159,7 +158,7 @@ public class PeerTypeRegistration implements TypeRegistration {
         // update a local map with the pdxtypes registered
         Object value = event.getNewValue();
         if (value instanceof PdxType) {
-          updateLocalMaps((PdxType) value);
+          updateClassToTypeMap((PdxType) value);
         }
       }
     });
@@ -351,25 +350,23 @@ public class PeerTypeRegistration implements TypeRegistration {
   public int defineType(PdxType newType) {
     verifyConfiguration();
     Integer existingId = typeToId.get(newType);
-
     if (existingId != null) {
       return existingId;
     }
     lock();
     try {
-      if (typeToId.isEmpty()) {
-        buildTypeToIdFromIdToType();
-      }
-      // double check if my type is in region in case the typeToId map has been updated while
-      // waiting to obtain a lock
-      existingId = typeToId.get(newType);
-      if (existingId != null) {
-        return existingId;
+      int id = getExistingIdForType(newType);
+      if (id != -1) {
+        return id;
       }
 
-      int id = allocateTypeId(newType);
+      id = allocateTypeId(newType);
       newType.setTypeId(id);
+
       updateIdToTypeRegion(newType);
+
+      typeToId.put(newType, id);
+
       return newType.getTypeId();
     } finally {
       unlock();
@@ -541,10 +538,12 @@ public class PeerTypeRegistration implements TypeRegistration {
     }
   }
 
-  private void buildTypeToIdFromIdToType() {
+  /** Should be called holding the dlock */
+  private int getExistingIdForType(PdxType newType) {
     int totalPdxTypeIdInDS = 0;
     TXStateProxy currentState = suspendTX();
     try {
+      int result = -1;
       for (Map.Entry<Object, Object> entry : getIdToType().entrySet()) {
         Object v = entry.getValue();
         Object k = entry.getKey();
@@ -558,16 +557,20 @@ public class PeerTypeRegistration implements TypeRegistration {
           int tmpDsId = PLACE_HOLDER_FOR_DS_ID & id;
           if (tmpDsId == this.dsId) {
             totalPdxTypeIdInDS++;
-            if (totalPdxTypeIdInDS >= this.maxTypeId) {
-              throw new InternalGemFireError(
-                  "Used up all of the PDX type ids for this distributed system. The maximum
number of PDX types is "
-                      + this.maxTypeId);
-            }
           }
 
           typeToId.put(foundType, id);
+          if (foundType.equals(newType)) {
+            result = foundType.getTypeId();
+          }
         }
       }
+      if (totalPdxTypeIdInDS == this.maxTypeId) {
+        throw new InternalGemFireError(
+            "Used up all of the PDX type ids for this distributed system. The maximum number
of PDX types is "
+                + this.maxTypeId);
+      }
+      return result;
     } finally {
       resumeTX(currentState);
     }
@@ -734,9 +737,8 @@ public class PeerTypeRegistration implements TypeRegistration {
   /**
    * adds a PdxType for a field to a {@code className => Set<PdxType>} map
    */
-  private void updateLocalMaps(PdxType type) {
+  private void updateClassToTypeMap(PdxType type) {
     if (type != null) {
-      typeToId.put(type, type.getTypeId());
       synchronized (this.classToType) {
         if (type.getClassName().equals(JSONFormatter.JSON_CLASSNAME)) {
           return; // no need to include here
@@ -805,9 +807,4 @@ public class PeerTypeRegistration implements TypeRegistration {
   public int getLocalSize() {
     return idToType.size();
   }
-
-  @VisibleForTesting
-  public int getTypeToIdSize() {
-    return typeToId.size();
-  }
 }


Mime
View raw message