From ec98d4cc43efea67f89a59f30fa6a374586473b4 Mon Sep 17 00:00:00 2001 From: Santhosh Gandhe <1909520+san81@users.noreply.github.com> Date: Thu, 24 Oct 2024 20:05:41 -0700 Subject: [PATCH] addressing review comments Signed-off-by: Santhosh Gandhe <1909520+san81@users.noreply.github.com> --- .../jira-source/build.gradle | 1 - .../source-crawler/build.gradle | 7 --- .../SaasPluginExecutorServiceProvider.java | 2 +- .../source/source_crawler/model/ItemInfo.java | 39 +++++++------- .../source_crawler/base/CrawlerTest.java | 22 +------- .../scheduler/LeaderSchedulerTest.java | 4 +- .../state/LeaderProgressStateTest.java | 2 +- .../source_crawler/model/ItemInfoTest.java | 39 -------------- .../source_crawler/model/TestItemInfo.java | 51 +++++++++++++++++++ 9 files changed, 74 insertions(+), 93 deletions(-) create mode 100644 data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/TestItemInfo.java diff --git a/data-prepper-plugins/saas-source-plugins/jira-source/build.gradle b/data-prepper-plugins/saas-source-plugins/jira-source/build.gradle index 6b36619abf..9979e0151d 100644 --- a/data-prepper-plugins/saas-source-plugins/jira-source/build.gradle +++ b/data-prepper-plugins/saas-source-plugins/jira-source/build.gradle @@ -16,7 +16,6 @@ dependencies { implementation 'com.fasterxml.jackson.core:jackson-databind' implementation 'javax.inject:javax.inject:1' implementation("org.springframework:spring-web:${libs.versions.spring.get()}") - implementation 'org.springframework.retry:spring-retry:1.3.4' implementation 'org.projectlombok:lombok:1.18.30' annotationProcessor 'org.projectlombok:lombok:1.18.30' diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/build.gradle b/data-prepper-plugins/saas-source-plugins/source-crawler/build.gradle index 09d2a659c3..892792011f 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/build.gradle +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/build.gradle @@ -1,6 +1,3 @@ -plugins { - id 'java-library' -} group = 'org.opensearch.dataprepper.plugins.source.source_crawler' @@ -28,7 +25,3 @@ dependencies { testImplementation 'com.fasterxml.jackson.datatype:jackson-datatype-jsr310' annotationProcessor 'org.projectlombok:lombok:1.18.30' } - -test { - useJUnitPlatform() -} \ No newline at end of file diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/SaasPluginExecutorServiceProvider.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/SaasPluginExecutorServiceProvider.java index bcb8f54acf..7dcbc4c6ab 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/SaasPluginExecutorServiceProvider.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/SaasPluginExecutorServiceProvider.java @@ -12,7 +12,7 @@ @Named public class SaasPluginExecutorServiceProvider { private static final Logger log = LoggerFactory.getLogger(SaasPluginExecutorServiceProvider.class); - public static final int DEFAULT_THREAD_COUNT = 50; + private static final int DEFAULT_THREAD_COUNT = 50; private final ExecutorService executorService; public SaasPluginExecutorServiceProvider() { diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfo.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfo.java index af29ee8ba9..d3e1b30a46 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfo.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/main/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfo.java @@ -1,29 +1,25 @@ package org.opensearch.dataprepper.plugins.source.source_crawler.model; -import lombok.Getter; -import lombok.NonNull; - import java.time.Instant; import java.util.Map; -@Getter -public abstract class ItemInfo { + +public interface ItemInfo { /** * Use this field to store primary item of a repository. Primary item of a repository is something - * which can be fetched/queried/obtained from repository just using its item ID. + * which can be fetched/queried/obtained from source service just using its item ID. */ - @NonNull - String itemId; + String getItemId(); /** * Use this field to store items metadata. Item metadata can be any information other than item * contents itself which can be used to apply regex filtering, change data capture etc. general * assumption here is that fetching metadata should be faster than fetching entire Item */ - Map metadata; + Map getMetadata(); /** * Process your change log events serially (preferably in a single thread) and ensure that you are @@ -31,19 +27,20 @@ public abstract class ItemInfo { * updates as it processes events out of order and it relies on this member to decide which change * log events to keep and which ones to discard. */ - Instant eventTime; + Instant getEventTime(); - public ItemInfo(String itemId) { - this.itemId = itemId; - } + String getPartitionKey(); - public ItemInfo(@NonNull String itemId, Map metadata, Instant eventTime) { - this.itemId = itemId; - this.metadata = metadata; - this.eventTime = eventTime; - } + /** + * Service specific Unique Id of this Item. + * @return String value indicating unique id of this item. + */ + String getId(); - public abstract String getPartitionKey(); - public abstract String getId(); - public abstract Map getKeyAttributes(); + /** + * Key attributes related to this Item. + * + * @return A map of key attributes of this Item. + */ + Map getKeyAttributes(); } diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/CrawlerTest.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/CrawlerTest.java index 6617fb590c..d474bfc373 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/CrawlerTest.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/CrawlerTest.java @@ -12,6 +12,7 @@ import org.opensearch.dataprepper.plugins.source.source_crawler.coordination.partition.SaasSourcePartition; import org.opensearch.dataprepper.plugins.source.source_crawler.coordination.state.SaasWorkerProgressState; import org.opensearch.dataprepper.plugins.source.source_crawler.model.ItemInfo; +import org.opensearch.dataprepper.plugins.source.source_crawler.model.TestItemInfo; import java.lang.reflect.Field; import java.time.Instant; @@ -157,27 +158,6 @@ private int getMaxItemsPerPage() throws NoSuchFieldException, IllegalAccessExcep return (int) maxItemsPerPageField.get(null); } - private static class TestItemInfo extends ItemInfo { - public TestItemInfo(String itemId, Map metadata, Instant eventTime) { - super(itemId, metadata, eventTime); - - } - @Override - public String getPartitionKey() { - return getItemId(); - } - - @Override - public String getId() { - return getItemId(); - } - - @Override - public Map getKeyAttributes() { - return new HashMap<>(); - } - } - private ItemInfo createTestItemInfo(String id, String created, String updated) { Map metadata = new HashMap<>(); if (created != null) metadata.put(Crawler.CREATED, created); diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/scheduler/LeaderSchedulerTest.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/scheduler/LeaderSchedulerTest.java index bb1910a448..44e7a6f5d2 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/scheduler/LeaderSchedulerTest.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/scheduler/LeaderSchedulerTest.java @@ -102,10 +102,10 @@ void testWhileLoopRunnningAfterTheSleep() throws InterruptedException { executorService.submit(leaderScheduler); //Wait for more than a minute as the default while loop wait time in leader scheduler is 1 minute - Thread.sleep(70000); + Thread.sleep(100); executorService.shutdownNow(); // Check if crawler was invoked and updated leader lease renewal time - verify(crawler, times(2)).crawl(Instant.ofEpochMilli(0L), coordinator); + verify(crawler, times(1)).crawl(Instant.ofEpochMilli(0L), coordinator); } } diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/state/LeaderProgressStateTest.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/state/LeaderProgressStateTest.java index 768f553e20..9986872907 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/state/LeaderProgressStateTest.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/coordination/state/LeaderProgressStateTest.java @@ -23,7 +23,7 @@ void testDefaultValues() throws JsonProcessingException { void testInitializedValues() throws JsonProcessingException { String state = "{\"last_poll_time\":1729391235717, \"initialized\": true}"; LeaderProgressState leaderProgressState = objectMapper.readValue(state, LeaderProgressState.class); - assert leaderProgressState.getLastPollTime() == Instant.ofEpochMilli(1729391235717L); + assert Instant.ofEpochMilli(1729391235717L).equals(leaderProgressState.getLastPollTime()); assert leaderProgressState.isInitialized(); } } diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfoTest.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfoTest.java index c92e07fb40..d92a6b639c 100644 --- a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfoTest.java +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/ItemInfoTest.java @@ -1,42 +1,13 @@ package org.opensearch.dataprepper.plugins.source.source_crawler.model; -import lombok.NonNull; import org.junit.jupiter.api.Test; import java.time.Instant; import java.util.Map; import java.util.UUID; -import static org.junit.jupiter.api.Assertions.assertThrows; - public class ItemInfoTest { - static class TestItemInfo extends ItemInfo { - - public TestItemInfo(@NonNull String itemId, Map metadata, Instant eventTime) { - super(itemId, metadata, eventTime); - } - - public TestItemInfo(String itemId) { - super(itemId); - } - - @Override - public String getPartitionKey() { - return "partitionKey"; - } - - @Override - public String getId() { - return "id"; - } - - @Override - public Map getKeyAttributes() { - return Map.of(); - } - } - @Test void testItemInfoSimpleConstructor() { String itemId = UUID.randomUUID().toString(); @@ -48,16 +19,6 @@ void testItemInfoSimpleConstructor() { assert itemInfo.getKeyAttributes().isEmpty(); } - @Test - void testItemInfoWithNullValues() { - assertThrows(NullPointerException.class, () -> new TestItemInfo(null, null, null)); - assertThrows(NullPointerException.class, () -> new TestItemInfo(null, null, Instant.ofEpochMilli(1234L)), - "itemId should not be null"); - assertThrows(NullPointerException.class, () -> new TestItemInfo("itemid", null, null), - "eventTime should not be null"); - - } - @Test void testItemInfo() { String itemId = UUID.randomUUID().toString(); diff --git a/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/TestItemInfo.java b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/TestItemInfo.java new file mode 100644 index 0000000000..03b0165f6f --- /dev/null +++ b/data-prepper-plugins/saas-source-plugins/source-crawler/src/test/java/org/opensearch/dataprepper/plugins/source/source_crawler/model/TestItemInfo.java @@ -0,0 +1,51 @@ +package org.opensearch.dataprepper.plugins.source.source_crawler.model; + +import java.time.Instant; +import java.util.Map; + +public class TestItemInfo implements ItemInfo { + + String itemId; + Map metadata; + Instant eventTime; + + public TestItemInfo(String itemId, Map metadata, Instant eventTime) { + this.itemId = itemId; + this.metadata = metadata; + this.eventTime = eventTime; + } + + public TestItemInfo(String itemId) { + this.itemId = itemId; + } + + @Override + public String getItemId() { + return itemId; + } + + @Override + public Map getMetadata() { + return this.metadata; + } + + @Override + public Instant getEventTime() { + return this.eventTime; + } + + @Override + public String getPartitionKey() { + return "partitionKey"; + } + + @Override + public String getId() { + return "id"; + } + + @Override + public Map getKeyAttributes() { + return Map.of(); + } +}