HTTP-63 caching for sync lookups #94

davidradl · 2024-05-08T16:45:36Z

Description

Introduce caching similar to the Flink JDBC connector, so long running synchronous Flink jobs can cache content resulting in less API calls.

Resolves #63

PR Checklist

Tests added
Changelog updated

davidradl · 2024-05-09T14:53:34Z

@kristoffSC @grzegorz8 the 1.18.1 job is failing with

14:50:32,522 [INFO] ------------------------------------------------------------------------
14:50:32,522 [INFO] BUILD SUCCESS
14:50:32,522 [INFO] ------------------------------------------------------------------------
14:50:32,523 [INFO] Total time:  02:01 min
14:50:32,523 [INFO] Finished at: 2024-05-09T14:50:32Z
14:50:32,523 [INFO] ------------------------------------------------------------------------
cat: target/site/jacoco/index.html: No such file or directory

Error: Process completed with exit code 1.

This looks like an infrastructure issue - as the build is successful then there is a cat error.

README.md

src/main/java/com/getindata/connectors/http/internal/table/lookup/HttpLookupTableSource.java

davidradl · 2024-05-10T09:51:45Z

@kristoffSC @grzegorz8 it looks like the build is successful but fails - is the machine running out of disk space / lost access to something maybe?

AdrianVasiliu

Minor inline comments.

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

src/main/java/com/getindata/connectors/http/internal/table/lookup/HttpLookupTableSource.java

pom.xml

CHANGELOG.md

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

README.md

kristoffSC · 2024-05-13T07:53:03Z

I will take a look why build fails.

davidradl · 2024-05-13T08:54:39Z

@AdrianVasiliu thank you very much for your detailed review, I really appreciate it. Great feedback :-)

davidradl · 2024-05-28T08:47:07Z

@kristoffSC a gentle nudge, we are looking to adopt this change, any news on the build issue?

MarekMaj · 2024-06-07T08:36:20Z

Hi @davidradl, we merged changes related to the build issue. Could you please rebase your commit?

davidradl · 2024-06-10T13:32:59Z

@MarekMaj thanks for making this change - it works for 2 of the releases bu not for 1.18.1, where I now see the CI fail with error

Run madrapps/[email protected]
Event is pull_request
base sha: 9bfbb4c8a1b722eac8dd1f1b1b71b1fcbf1f02ce
head sha: 18f2d82ac13333344bacd4a02e7e2f486a649ae6
node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[Error: ENOENT: no such file or directory, open '/home/runner/work/flink-http-connector/flink-http-connector/target/site/jacoco/jacoco.xml'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/runner/work/flink-http-connector/flink-http-connector/target/site/jacoco/jacoco.xml'

}

I suspect this is related to your recent jakoco commit

MarekMaj · 2024-06-11T12:53:58Z

@davidradl indeed, for PR created from forked repositories it caused problems due to insufficient permissions. I tested build again using forked repo, please rebase your code again:)

Signed-off-by: David Radley <[email protected]>

MarekMaj · 2024-06-12T12:18:18Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+
+    private transient PollingClient<RowData> client;
+
+    private LookupCache cache;


Cache is not used in this function

yep, seems like you would like to use it in lookup(RowData keyRow) method @davidradl
Also, the cache field can be declared as final.

src/main/java/com/getindata/connectors/http/internal/table/lookup/HttpLookupTableSource.java

MarekMaj · 2024-06-12T12:29:33Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/HttpLookupTableSource.java

+                            cache
+                    );
+            if (cache != null) {
+                log.debug("PartialCachingLookupProvider; cache = " + cache);


Please use logger placeholders like described in another comment

MarekMaj · 2024-06-12T12:31:05Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+     */
+    @Override
+    public Collection<RowData> lookup(RowData keyRow) throws IOException {
+        log.debug("lookup=" + lookupRow);


Please use logger placeholders instead of explicit string concatenation to avoid calling toString method when logger is disabled for the given logging level log.debug("lookupRow = {}", lookupRow);

MarekMaj · 2024-06-12T12:31:24Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+        Optional<RowData> rowData=  client.pull(keyRow);
+        List<RowData> result = new ArrayList<>();
+        rowData.ifPresent(row -> { result.add(row); });
+        log.debug("lookup result=" + result);


Please use logger placeholders like described in another comment

kristoffSC · 2024-06-12T19:29:11Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+
+    private transient PollingClient<RowData> client;
+
+    private LookupCache cache;


yep, seems like you would like to use it in lookup(RowData keyRow) method @davidradl
Also, the cache field can be declared as final.

kristoffSC · 2024-06-12T19:32:29Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+        context
+            .getMetricGroup()
+            .gauge("http-table-lookup-call-counter", () -> localHttpCallCounter.intValue());
+    }


remember to call cache.open(...) here

kristoffSC · 2024-06-12T19:33:40Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+        localHttpCallCounter.incrementAndGet();
+        Optional<RowData> rowData=  client.pull(keyRow);
+        List<RowData> result = new ArrayList<>();
+        rowData.ifPresent(row -> { result.add(row); });


nit:
rowData.ifPresent(result::add);

kristoffSC · 2024-06-12T19:49:05Z

src/main/java/com/getindata/connectors/http/internal/table/lookup/HttpLookupTableSource.java

+                            lookupConfig,
+                            cache
+                    );
+            if (cache != null) {


I'm not super happy that we can pass null as cache to HttpLookupSource. Plus if cache is null you still create an instance of CachingHttpTableLookupFunction.

I'm also thinking whether cache should be passed to HttpLookupTableSource
We have HttpLookupConfig that contains entire config (via ReadableConfig) that is already passed to HttpLookupTableSource. You could move getLookupCache from HttpLookupTableSourceFactory to HttpLookupTableSource and create cache there.

Going step futher, you could have two implementations for HttpLookupTableSource -> cached and noCache, build a factory class for it and move getLookupCache to that factory

davidradl · 2024-06-24T09:38:38Z

@kristoffSC @MarekMaj hope to get to this this week. Thanks for your feedback.

grzegorz8 · 2024-07-25T11:59:57Z

@kristoffSC @MarekMaj hope to get to this this week. Thanks for your feedback.

Hey @davidradl! Do you plan to continue working on the PR anytime soon? I can take over the PR if you are busy right now.

grzegorz8 · 2024-07-29T10:09:26Z

...java/com/getindata/connectors/http/internal/table/lookup/CachingHttpTableLookupFunction.java

+import com.getindata.connectors.http.internal.utils.SerializationSchemaUtils;
+
+@Slf4j
+public class CachingHttpTableLookupFunction extends LookupFunction {


I wonder if we need this class at all. For example in flink-connector-jdbc the same lookup function is used for caching and non-caching approach: https://github.com/apache/flink-connector-jdbc/blob/v3.2/flink-connector-jdbc/src/main/java/org/apache/flink/connector/jdbc/table/JdbcDynamicTableSource.java#L96-L121

Refer to the comment #94 (comment)

kristoffSC · 2024-08-01T20:25:17Z

Done in #109

davidradl mentioned this pull request May 8, 2024

HTTP lookup source max retries #90

Closed

2 tasks

davidradl force-pushed the http63 branch from c59c984 to 28ec114 Compare May 9, 2024 14:48

davidradl marked this pull request as ready for review May 9, 2024 14:52

davidradl force-pushed the http63 branch from 28ec114 to 272f04a Compare May 9, 2024 15:07

kristoffSC changed the title ~~HTTP63 caching for sync lookups~~ HTTP-63 caching for sync lookups May 10, 2024

kristoffSC self-requested a review May 10, 2024 07:30

kristoffSC self-assigned this May 10, 2024

kristoffSC added the enhancement New feature or request label May 10, 2024

kristoffSC reviewed May 10, 2024

View reviewed changes

davidradl force-pushed the http63 branch from 272f04a to 312071b Compare May 10, 2024 09:18

AdrianVasiliu reviewed May 10, 2024

View reviewed changes

AdrianVasiliu reviewed May 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

davidradl force-pushed the http63 branch from 312071b to e633b67 Compare May 13, 2024 08:52

davidradl force-pushed the http63 branch from e633b67 to 18f2d82 Compare June 10, 2024 13:26

HTTP63 HTTP63 caching for synchronous lookups

5506cb3

Signed-off-by: David Radley <[email protected]>

davidradl force-pushed the http63 branch from 18f2d82 to 5506cb3 Compare June 12, 2024 09:49

MarekMaj reviewed Jun 12, 2024

View reviewed changes

kristoffSC requested changes Jun 12, 2024

View reviewed changes

grzegorz8 reviewed Jul 29, 2024

View reviewed changes

grzegorz8 mentioned this pull request Jul 29, 2024

HTTP-63 Add caching #109

Merged

2 tasks

kristoffSC closed this Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTP-63 caching for sync lookups #94

HTTP-63 caching for sync lookups #94

davidradl commented May 8, 2024 •

edited by kristoffSC

Loading

davidradl commented May 9, 2024 •

edited

Loading

davidradl commented May 10, 2024

AdrianVasiliu left a comment

kristoffSC commented May 13, 2024

davidradl commented May 13, 2024

davidradl commented May 28, 2024

MarekMaj commented Jun 7, 2024

davidradl commented Jun 10, 2024 •

edited

Loading

MarekMaj commented Jun 11, 2024

MarekMaj Jun 12, 2024

kristoffSC Jun 12, 2024

MarekMaj Jun 12, 2024

MarekMaj Jun 12, 2024

MarekMaj Jun 12, 2024

kristoffSC Jun 12, 2024

kristoffSC Jun 12, 2024

kristoffSC Jun 12, 2024

kristoffSC Jun 12, 2024

davidradl commented Jun 24, 2024

grzegorz8 commented Jul 25, 2024

grzegorz8 Jul 29, 2024

MarekMaj Jul 29, 2024

kristoffSC commented Aug 1, 2024


		private transient PollingClient<RowData> client;

		private LookupCache cache;

HTTP-63 caching for sync lookups #94

HTTP-63 caching for sync lookups #94

Conversation

davidradl commented May 8, 2024 • edited by kristoffSC Loading

Description

PR Checklist

davidradl commented May 9, 2024 • edited Loading

davidradl commented May 10, 2024

AdrianVasiliu left a comment

Choose a reason for hiding this comment

kristoffSC commented May 13, 2024

davidradl commented May 13, 2024

davidradl commented May 28, 2024

MarekMaj commented Jun 7, 2024

davidradl commented Jun 10, 2024 • edited Loading

MarekMaj commented Jun 11, 2024

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

davidradl commented Jun 24, 2024

grzegorz8 commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoffSC commented Aug 1, 2024

davidradl commented May 8, 2024 •

edited by kristoffSC

Loading

davidradl commented May 9, 2024 •

edited

Loading

davidradl commented Jun 10, 2024 •

edited

Loading