Fixes performance regression with JacksonEvent put/delete operations. (…

…opensearch-project#4650) With the addition of the EventKey, JacksonEvent always creates a JacksonEventKey in order to use the same code for all paths. However, when put/delete calls are made with a String key, JacksonEvent does not need the JSON Pointer. But, it is created anyway. This adds more work to the put/delete calls that have not yet migrated to the String version. This fixes regression by adding a lazy initialization option when used in JacksonEvent. We should not be lazy when used with the EventKeyFactory since we may lose some up-front validations. Signed-off-by: David Venable <[email protected]>
sb2k16 · Jun 21, 2024 · b4b71e2 · b4b71e2
1 parent 0aba83b
commit b4b71e2
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 17 deletions.
diff --git a/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java b/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEvent.java
@@ -170,7 +170,7 @@ public void put(EventKey key, Object value) {
      */
     @Override
     public void put(final String key, final Object value) {
-        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.PUT);
+        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.PUT);
         put(jacksonEventKey, value);
     }
 
@@ -229,7 +229,7 @@ private static JacksonEventKey asJacksonEventKey(EventKey key) {
      */
     @Override
     public <T> T get(final String key, final Class<T> clazz) {
-        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.GET);
+        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.GET);
         return get(jacksonEventKey, clazz);
     }
 
@@ -274,7 +274,7 @@ public <T> List<T> getList(EventKey key, Class<T> clazz) {
      */
     @Override
     public <T> List<T> getList(final String key, final Class<T> clazz) {
-        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.GET);
+        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.GET);
         return getList(jacksonEventKey, clazz);
     }
 
@@ -330,7 +330,7 @@ public void delete(final EventKey key) {
      */
     @Override
     public void delete(final String key) {
-        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.DELETE);
+        final JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.DELETE);
         delete(jacksonEventKey);
     }
 
@@ -362,7 +362,7 @@ public String getAsJsonString(EventKey key) {
 
     @Override
     public String getAsJsonString(final String key) {
-        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.GET);
+        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.GET);
         return getAsJsonString(jacksonEventKey);
     }
 
@@ -459,7 +459,7 @@ public boolean containsKey(EventKey key) {
 
     @Override
     public boolean containsKey(final String key) {
-        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.GET);
+        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.GET);
         return containsKey(jacksonEventKey);
     }
 
@@ -474,7 +474,7 @@ public boolean isValueAList(EventKey key) {
 
     @Override
     public boolean isValueAList(final String key) {
-        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, EventKeyFactory.EventAction.GET);
+        JacksonEventKey jacksonEventKey = new JacksonEventKey(key, true, EventKeyFactory.EventAction.GET);
         return isValueAList(jacksonEventKey);
     }
 

diff --git a/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEventKey.java b/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/event/JacksonEventKey.java
@@ -22,14 +22,45 @@ class JacksonEventKey implements EventKey {
     private final String key;
     private final EventKeyFactory.EventAction[] eventActions;
     private final String trimmedKey;
-    private final List<String> keyPathList;
-    private final JsonPointer jsonPointer;
+    private List<String> keyPathList;
+    private JsonPointer jsonPointer;
     private final Set<EventKeyFactory.EventAction> supportedActions;
 
+    /**
+     * Constructor for the JacksonEventKey which should only be used by implementation
+     * of {@link EventKeyFactory} in Data Prepper core.
+     *
+     * @param key The key
+     * @param eventActions Event actions to support
+     */
     JacksonEventKey(final String key, final EventKeyFactory.EventAction... eventActions) {
+        this(key, false, eventActions);
+    }
+
+    /**
+     * Constructs a new JacksonEventKey.
+     * <p>
+     * This overload should only be used by {@link JacksonEvent} directly. It allows for skipping creating
+     * some resources knowing they will not be needed. The {@link JacksonEvent} only needs a JSON pointer
+     * when performing GET event actions. So we can optimize PUT/DELETE actions when called with a string
+     * key instead of an EventKey by not creating the JSON Pointer at all.
+     * <p>
+     * For EventKey's constructed through the factory, we should not perform lazy initialization since
+     * we may lose some possible validations.
+     *
+     * @param key the key
+     * @param lazy Use true to lazily initialize. This will not be thread-safe, however.
+     * @param eventActions Event actions to support
+     */
+    JacksonEventKey(final String key, final boolean lazy, final EventKeyFactory.EventAction... eventActions) {
         this.key = Objects.requireNonNull(key, "Parameter key cannot be null for EventKey.");
         this.eventActions = eventActions.length == 0 ? new EventKeyFactory.EventAction[] { EventKeyFactory.EventAction.ALL } : eventActions;
 
+        supportedActions = EnumSet.noneOf(EventKeyFactory.EventAction.class);
+        for (final EventKeyFactory.EventAction eventAction : this.eventActions) {
+            supportedActions.addAll(eventAction.getSupportedActions());
+        }
+
         if(key.isEmpty()) {
             for (final EventKeyFactory.EventAction action : this.eventActions) {
                 if (action.isMutableAction()) {
@@ -40,14 +71,10 @@ class JacksonEventKey implements EventKey {
 
         trimmedKey = checkAndTrimKey(key);
 
-        keyPathList = Collections.unmodifiableList(Arrays.asList(trimmedKey.split(SEPARATOR, -1)));
-        jsonPointer = toJsonPointer(trimmedKey);
-
-        supportedActions = EnumSet.noneOf(EventKeyFactory.EventAction.class);
-        for (final EventKeyFactory.EventAction eventAction : this.eventActions) {
-            supportedActions.addAll(eventAction.getSupportedActions());
+        if(!lazy) {
+            keyPathList = toKeyPathList();
+            jsonPointer = toJsonPointer(trimmedKey);
         }
-
     }
 
     @Override
@@ -60,10 +87,16 @@ String getTrimmedKey() {
     }
 
     List<String> getKeyPathList() {
+        if(keyPathList == null) {
+            keyPathList = toKeyPathList();
+        }
         return keyPathList;
     }
 
     JsonPointer getJsonPointer() {
+        if(jsonPointer == null) {
+            jsonPointer = toJsonPointer(trimmedKey);
+        }
         return jsonPointer;
     }
 
@@ -136,7 +169,11 @@ private static boolean isValidKey(final String key) {
         return true;
     }
 
-    private JsonPointer toJsonPointer(final String key) {
+    private List<String> toKeyPathList() {
+        return Collections.unmodifiableList(Arrays.asList(trimmedKey.split(SEPARATOR, -1)));
+    }
+
+    private static JsonPointer toJsonPointer(final String key) {
         final String jsonPointerExpression;
         if (key.isEmpty() || key.startsWith("/")) {
             jsonPointerExpression = key;

diff --git a/...prepper-api/src/test/java/org/opensearch/dataprepper/model/event/JacksonEventKeyTest.java b/...prepper-api/src/test/java/org/opensearch/dataprepper/model/event/JacksonEventKeyTest.java
@@ -115,6 +115,23 @@ void getJsonPointer_returns_the_same_instance_for_multiple_calls() {
         assertThat(objectUnderTest.getJsonPointer(), sameInstance(jsonPointer));
     }
 
+    @ParameterizedTest
+    @EnumSource(value = EventKeyFactory.EventAction.class)
+    void getJsonPointer_returns_valid_JsonPointer_when_constructed_with_fromJacksonEvent(final EventKeyFactory.EventAction eventAction) {
+        final String testKey = UUID.randomUUID().toString();
+        final JacksonEventKey objectUnderTest = new JacksonEventKey(testKey, true, eventAction);
+
+        final JsonPointer jsonPointer = objectUnderTest.getJsonPointer();
+        assertThat(jsonPointer, notNullValue());
+        assertThat(jsonPointer.toString(), equalTo("/" + testKey));
+    }
+
+    @ParameterizedTest
+    @ArgumentsSource(KeyPathListArgumentsProvider.class)
+    void getKeyPathList_returns_expected_value_when_constructed_with_fromJacksonEvent(final String key, final List<String> expectedKeyPathList) {
+        assertThat(new JacksonEventKey(key, true).getKeyPathList(), equalTo(expectedKeyPathList));
+    }
+
     @ParameterizedTest
     @ArgumentsSource(SupportsArgumentsProvider.class)
     void supports_returns_true_if_any_supports(final List<EventKeyFactory.EventAction> eventActionsList, final EventKeyFactory.EventAction otherAction, final boolean expectedSupports) {