SaaS Crawler Module #5095

san81 · 2024-10-21T19:15:22Z

Description

Introducing SaaS Source Plugins module and a base Jira Source plugin class

Issues Resolved

Resolves #4754

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…odule for all of the gradle sources Signed-off-by: Santhosh Gandhe <[email protected]>

Signed-off-by: Santhosh Gandhe <[email protected]>

Signed-off-by: Maxwell Brown <[email protected]>

Signed-off-by: Santhosh Gandhe <[email protected]>

full test coverage for base folder, spotless fixes

Signed-off-by: Santhosh Gandhe <[email protected]>

…ources-module

.gitignore

dlvenable · 2024-10-22T15:48:09Z

data-prepper-plugins/saas-source-plugins/jira-source/build.gradle

+    implementation 'com.fasterxml.jackson.core:jackson-core'
+    implementation 'com.fasterxml.jackson.core:jackson-databind'
+    implementation 'com.mashape.unirest:unirest-java:1.4.9'
+    implementation 'com.google.code.gson:gson:2.8.9'


Do we need this dependency? Please remove if possible.

Removed them

data-prepper-plugins/saas-source-plugins/jira-source/build.gradle

dlvenable · 2024-10-22T15:49:15Z

data-prepper-plugins/saas-source-plugins/jira-source/build.gradle

+    implementation 'org.projectlombok:lombok:1.18.30'
+    annotationProcessor 'org.projectlombok:lombok:1.18.30'
+
+    testImplementation platform('org.junit:junit-bom:5.10.0')


You don't need either of these two lines.

removed them

dlvenable · 2024-10-22T15:49:47Z

data-prepper-plugins/saas-source-plugins/saas-crawler/build.gradle

+    enabled = false
+}
+
+repositories {


Please remove this block. You don't need it.

.../opensearch/dataprepper/plugins/source/saas/crawler/SaasCrawlerApplicationContextMarker.java

dlvenable · 2024-10-22T15:53:01Z

...ensearch/dataprepper/plugins/source/saas/crawler/base/SaasPluginExecutorServiceProvider.java

+
+@Named
+public class SaasPluginExecutorServiceProvider {
+    Logger  log = LoggerFactory.getLogger(SaasPluginExecutorServiceProvider.class);


This should be private static final

Addressed it

...ensearch/dataprepper/plugins/source/saas/crawler/base/SaasPluginExecutorServiceProvider.java

.../main/java/org/opensearch/dataprepper/plugins/source/saas/crawler/base/SaasSourceConfig.java

settings.gradle

… the review input Signed-off-by: Santhosh Gandhe <[email protected]>

graytaylor0 · 2024-10-22T20:32:31Z

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas/crawler/base/Crawler.java

+        Iterator<ItemInfo> itemInfoIterator = client.listItems();
+        log.info("Starting to crawl the source");
+        long updatedPollTime = 0;
+        log.info("Creating Partitions");


There are a log of logs throughout the PR that are info logs that look like they should be debug

Adjusted the log level for some and removed a few log statements.

graytaylor0 · 2024-10-22T20:34:34Z

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas/crawler/base/Crawler.java

+                updatedPollTime = Math.max(updatedPollTime, niUpdated);
+                log.info("updated poll time {}", updatedPollTime);
+            }
+            createPartition(itemInfoList, coordinator);


Why are we passing a full list to this method? Can we just create each partition inline or do they all need to be stored first?

We are passing maxItemsPerPage number of items to this method. i.e. the page size in our paginated crawling. All the items in this page will go into one partition (or a work item). Like a partition per page.

graytaylor0 · 2024-10-22T20:37:01Z

...va/org/opensearch/dataprepper/plugins/source/saas/crawler/coordination/PartitionFactory.java

+        } else {
+            // Unable to acquire other partitions.
+            // Probably we will introduce Global state in the future but for now, we don't expect to reach here.
+             throw new RuntimeException("Unable to acquire other partitions. " +


Maybe print out the partitionType here if we get to this point.

Added to the exception message.

graytaylor0 · 2024-10-22T20:51:33Z

...ensearch/dataprepper/plugins/source/saas/crawler/coordination/scheduler/LeaderScheduler.java

+                if(leaderPartition != null) {
+                    // Extend the timeout
+                    // will always be a leader until shutdown
+                    coordinator.saveProgressStateForPartition(leaderPartition, Duration.ofMinutes(DEFAULT_EXTEND_LEASE_MINUTES));


Should catch exceptions that can come from this call so your thread doesn't shut down.

Wrapped this statement around try catch now. I don't see that this method is throwing any exception though!

It won't most of the time but it can. This was a bug in Dynamo at one time (#4850)

dynamo store hit a 5xx

Thank you for clarifying 👍

Signed-off-by: Santhosh Gandhe <[email protected]>

graytaylor0 · 2024-10-23T17:53:41Z

.../main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/SaasSourcePlugin.java

+    this.buffer = buffer;
+
+    boolean isPartitionCreated = coordinator.createPartition(new LeaderPartition());
+    log.info("Leader partition creation status: {}", isPartitionCreated);


This will result in one of these logs whenever a new data prepper instance starts

Leader partition creation status: false

and isn't really helpful. Can be debug

graytaylor0 · 2024-10-23T17:55:52Z

...ensearch/dataprepper/plugins/source/saas_crawler/coordination/scheduler/WorkerScheduler.java

+                    processPartition(partition.get(), buffer, sourceConfig);
+
+                } else {
+                    log.info("No partition available. Going to Sleep for a while ");


This may also be a little noisy. Maybe a metric tracking this would be better?

Signed-off-by: Santhosh Gandhe <[email protected]>

kkondaka · 2024-10-23T22:51:21Z

...er/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/SaasClient.java

+    Iterator<ItemInfo> listItems();
+
+
+    void setLastPollTime(long lastPollTime);


Please add javadoc comments for all API

kkondaka · 2024-10-23T22:54:03Z

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/Crawler.java

@@ -0,0 +1,81 @@
+package org.opensearch.dataprepper.plugins.source.saas_crawler.base;


Do you not see a need for common interface for all crawlers? I was thinking there would be an interface with some default implementation and so on.

Crawler relay on a source plugin specific iterator implementation and dispatch the work to source plugin specific client implementation. Crawler itself has generic logic for pagination.

kkondaka · 2024-10-23T22:56:44Z

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/Crawler.java

+                }
+                itemInfoList.add(nextItem);
+                Map<String, String> metadata = nextItem.getMetadata();
+                long niCreated = Long.parseLong(metadata.get(CREATED)!=null? metadata.get(CREATED):"0");


Should the fallback value of 0 or current time?

kkondaka · 2024-10-23T22:57:24Z

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/Crawler.java

+            createPartition(itemInfoList, coordinator);
+        }while (itemInfoIterator.hasNext());
+        log.debug("Crawling completed in {} ms", System.currentTimeMillis() - startTime);
+        return updatedPollTime != 0 ? updatedPollTime : startTime;


Oh, looks like it is falling back to current time here.

kkondaka · 2024-10-23T23:04:58Z

.../main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/SaasSourcePlugin.java

+  private final Crawler crawler;
+
+
+  @DataPrepperPluginConstructor


I do not think you use @DataPrepperPluginConstructor for abstract source classes. see ./data-prepper-plugins/http-source-common/src/main/java/org/opensearch/dataprepper/http/BaseHttpSource.java

Agree. No use of this annotation here. Removed it.

kkondaka · 2024-10-23T23:06:31Z

.../main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/SaasSourcePlugin.java

+
+  @Override
+  public boolean areAcknowledgementsEnabled() {
+    return Source.super.areAcknowledgementsEnabled();


Not a good idea. This should be left to the derived classes.

Agree. Removed this method. Each source plugin will implement their own version.

kkondaka · 2024-10-23T23:10:16Z

...ler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/model/ItemInfo.java

+     * 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<String, String> metadata;


Probably better to make it Map<String, Object>, it is probably unrealistic to expect all of metadata values to be Strings.

Considering each source may have different needs, agree to make it more generic. Converted the map as suggested.

Signed-off-by: Santhosh Gandhe <[email protected]>

...ensearch/dataprepper/plugins/source/saas_crawler/coordination/state/LeaderProgressState.java

settings.gradle

...awler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/base/Crawler.java

...ler/src/main/java/org/opensearch/dataprepper/plugins/source/saas_crawler/model/ItemInfo.java

...ensearch/dataprepper/plugins/source/saas_crawler/base/SaasPluginExecutorServiceProvider.java

data-prepper-plugins/saas-source-plugins/saas-crawler/build.gradle

...ira-source/src/main/java/org/opensearch/dataprepper/plugins/source/saas/jira/JiraSource.java

data-prepper-plugins/saas-source-plugins/jira-source/build.gradle

dlvenable · 2024-10-25T21:13:29Z

...ain/java/org/opensearch/dataprepper/plugins/source/source_crawler/base/SaasSourcePlugin.java

+ * JiraConnector connector entry point.
+ */
+
+public abstract class SaasSourcePlugin implements Source<Record<Event>>, UsesEnhancedSourceCoordination {


Let's also rename this to SourceCrawler.

...ensearch/dataprepper/plugins/source/saas_crawler/coordination/state/LeaderProgressState.java

dlvenable

Thank you @san81 for the improvements. This is very good. I think just a few more changes and I'll be good to go.

dlvenable · 2024-10-25T21:22:12Z

...ch/dataprepper/plugins/source/source_crawler/coordination/scheduler/WorkerSchedulerTest.java

+        executorService.submit(workerScheduler);
+
+        //Wait for more than a minute as the default while loop wait time in leader scheduler is 1 minute
+        Thread.sleep(11000);


We still need to lower this sleep.

dlvenable · 2024-10-25T21:22:23Z

...ch/dataprepper/plugins/source/source_crawler/coordination/scheduler/WorkerSchedulerTest.java

+        executorService.submit(workerScheduler);
+
+        //Wait for more than a minute as the default while loop wait time in leader scheduler is 1 minute
+        Thread.sleep(11000);


We still need to lower this sleep.

dlvenable

I'm ok with getting the other changes in a follow-on PR.

Signed-off-by: Santhosh Gandhe <[email protected]> converting last_poll_time to java Instant type Signed-off-by: Santhosh Gandhe <[email protected]> we are now capturing Crawling times Signed-off-by: Santhosh Gandhe <[email protected]> ItemInfo long timestamp is now using Instant type Signed-off-by: Santhosh Gandhe <[email protected]> addressing review comments Signed-off-by: Santhosh Gandhe <[email protected]> Instant conversion Signed-off-by: Santhosh Gandhe <[email protected]> addressing review comments Signed-off-by: Santhosh Gandhe <[email protected]> code formatting Signed-off-by: Santhosh Gandhe <[email protected]> removed long polling by enabling setter on the leader scheduler timer Signed-off-by: Santhosh Gandhe <[email protected]> reducing wait times Signed-off-by: Santhosh Gandhe <[email protected]>

san81 added 2 commits October 21, 2024 12:04

Introducing SaaS sources gradle module and SaaS crawler as a common m…

bc24971

…odule for all of the gradle sources Signed-off-by: Santhosh Gandhe <[email protected]>

test classes

e257f31

Signed-off-by: Santhosh Gandhe <[email protected]>

san81 requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, KarstenSchnitter, dlvenable and oeyh as code owners October 21, 2024 19:15

san81 and others added 8 commits October 21, 2024 12:32

Plain empty Jira Source plugin

b238ae9

Signed-off-by: Santhosh Gandhe <[email protected]>

Parition Factory Tests

696c1a5

Signed-off-by: Santhosh Gandhe <[email protected]>

additional tests

439ef92

Signed-off-by: Santhosh Gandhe <[email protected]>

additional tests

1a76aac

Signed-off-by: Santhosh Gandhe <[email protected]>

full test coverage for base folder, spotless fixes

44828f8

Signed-off-by: Maxwell Brown <[email protected]>

additional tests

0881b43

Signed-off-by: Santhosh Gandhe <[email protected]>

Merge pull request #6 from Galactus22625/Saas-Crawler-Base-UnitTests

26137ea

full test coverage for base folder, spotless fixes

additional test coverage

f535b32

Signed-off-by: Santhosh Gandhe <[email protected]>

san81 changed the title ~~Saas sources module~~ Saas Crawler Module Oct 22, 2024

san81 changed the title ~~Saas Crawler Module~~ SaaS Crawler Module Oct 22, 2024

san81 added 3 commits October 22, 2024 07:30

Merge branch 'opensearch-project:main' into saas-sources-module

7096f1c

merging main

b90b929

Signed-off-by: Santhosh Gandhe <[email protected]>

Merge remote-tracking branch 'origin/saas-sources-module' into saas-s…

eccabb8

…ources-module

dlvenable requested changes Oct 22, 2024

View reviewed changes

addressing review comments and also package name refactoring based on…

86338bf

… the review input Signed-off-by: Santhosh Gandhe <[email protected]>

graytaylor0 reviewed Oct 22, 2024

View reviewed changes

more review comments

65bc47b

Signed-off-by: Santhosh Gandhe <[email protected]>

san81 requested a review from sb2k16 as a code owner October 22, 2024 22:40

san81 added 2 commits October 22, 2024 16:21

adjusted the log level and removed unwanted log messages

31ab7b9

Signed-off-by: Santhosh Gandhe <[email protected]>

small clean ups

8af4240

Signed-off-by: Santhosh Gandhe <[email protected]>

test case assertion fix

9a6fda7

Signed-off-by: Santhosh Gandhe <[email protected]>

san81 requested a review from dlvenable October 23, 2024 00:18

better coverage

cb3d94f

Signed-off-by: Santhosh Gandhe <[email protected]>

graytaylor0 previously approved these changes Oct 23, 2024

View reviewed changes

step down the log level based on the review comments

c5af55a

Signed-off-by: Santhosh Gandhe <[email protected]>

san81 dismissed graytaylor0’s stale review via c5af55a October 23, 2024 18:00

san81 added 2 commits October 23, 2024 11:00

Merge branch 'opensearch-project:main' into saas-sources-module

647ba10

taking the coverage to 100%

5a4e966

Signed-off-by: Santhosh Gandhe <[email protected]>

kkondaka reviewed Oct 23, 2024

View reviewed changes

addressing review comments

cf35364

Signed-off-by: Santhosh Gandhe <[email protected]>

dlvenable requested changes Oct 24, 2024

View reviewed changes

dlvenable reviewed Oct 25, 2024

View reviewed changes

dlvenable requested changes Oct 25, 2024

View reviewed changes

san81 requested a review from dlvenable October 25, 2024 21:26

dlvenable approved these changes Oct 25, 2024

View reviewed changes

san81 force-pushed the saas-sources-module branch from 0087491 to 39d98e7 Compare October 25, 2024 23:46

kkondaka approved these changes Oct 28, 2024

View reviewed changes

kkondaka merged commit 675864d into opensearch-project:main Oct 28, 2024
72 of 74 checks passed

dlvenable mentioned this pull request Jan 14, 2025

Welcoming Santhosh Gandhe (san81) to the Data Prepper maintainers. #5329

Merged

4 tasks

		Iterator<ItemInfo> listItems();


		void setLastPollTime(long lastPollTime);

		@@ -0,0 +1,81 @@
		package org.opensearch.dataprepper.plugins.source.saas_crawler.base;

SaaS Crawler Module #5095

SaaS Crawler Module #5095

Conversation

san81 commented Oct 21, 2024 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

san81 commented Oct 21, 2024 •

edited

Loading