refactor: cache transformations in PersistentQueryMetadata #3708

vpapavas · 2019-10-31T00:10:05Z

Description

Addresses #3542 . Don't build transformations for every request on the same state store but rather built them only the first time.

Two follow up issues: #3855 and #3854

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

ghost · 2019-10-31T00:10:08Z

@confluentinc It looks like @vpapavas just signed our Contributor License Agreement. 👍

Always at your service,

clabot

vinothchandar

Tracing through the code, this LGTM. Persistent queries are registered once at EngineExecutor::executePersistentQuery and the queryMetadata is held in the hashmap.

engineContext.registerQuery(queryMetadata); So should be safe to cache this.

Good to get one more review from @rodesai or @agavra as well. And may be add a test to check the materialization is cached (this way if we accidentally undo this caching later on, it won't silently fail)

agavra · 2019-10-31T17:04:13Z

ksql-engine/src/main/java/io/confluent/ksql/util/PersistentQueryMetadata.java

@@ -134,6 +137,10 @@ public PhysicalSchema getPhysicalSchema() {
      final QueryId queryId,
      final QueryContext.Stacker contextStacker
  ) {
-    return materializationProvider.map(builder -> builder.build(queryId, contextStacker));
+    if (!materialization.isPresent()) {
+      materialization = materializationProvider.map(builder ->


it looks like we're caching a value that may not have the same queyId that the caller is making (i.e. the first time I call it with queryId=123 and second time I call it with queryId=124, it will use 123 for both of them) - digging into the code, it looks like it might cause our logging pipeline to be a little messed up. We should look at this with a critical eye and see what we can do

vpapavas · 2019-11-01T00:29:33Z

It seems that the QueryId is only used for the ProcessingLogger which is used to log errors. This logger is used for writing record processing log messages. Is this something we want to do for pull queries?

vinothchandar · 2019-11-01T12:43:56Z

good catch @agavra .. One way to fix could be to to pull queryID out of the materialization itself and just pass it in during execution..

rodesai · 2019-11-01T16:26:03Z

good catch @agavra .. One way to fix could be to to pull queryID out of the materialization itself and just pass it in during execution..

Another way of looking at it would be (if we think that what's really expensive here is codegen) to pull out all the codegen stuff and just do that up front. For example, for projections store the result of codegen (List<SelectExpression>) in ProjectInfo, and have SelectValueMapperFactory take a List<SelectInfo> instead of List<SelectExpression>.

vpapavas · 2019-11-01T18:52:07Z

For streaming queries, it makes sense every query to get a different id and to write to a different logger. For pull queries, since they are one-off queries, it doesn't make sense to create random id and write every time to a different logger. That's why I create one queryId per sink and all pull queries to that sink share the same id and hence write to the same logger. I synchronized the access to the logger for multi-threading.

ksql-engine/src/main/java/io/confluent/ksql/util/PersistentQueryMetadata.java

...cution/src/main/java/io/confluent/ksql/execution/util/EngineProcessingLogMessageFactory.java

the bug I identified is no longer present

vpapavas · 2019-11-04T18:51:59Z

Hey @big-andy-coates , what do you think about this? Do you have a different suggestion?

big-andy-coates

Thanks @vpapavas

As I comment below, I think a better approach is to pull the queryId out of the mappers and filters etc, so that it can be passed in as a parameter when being executed. This would allow us to create the baked code as we're building the source aggregate query, i.e. rather than build the materialization, and cache it, on first use, we just build it when building the source aggregate query and store it, rather than the provider, in the query metadata.

Are you up for giving that a go? Maybe leave this PR as is and have a go at creating a new one with the above approach. Then if we're stuck for time we can look to put this one if for the initial launch and then switch to the better approach later.

big-andy-coates · 2019-11-06T13:02:20Z

ksql-engine/src/test/java/io/confluent/ksql/query/QueryExecutorTest.java

@@ -310,20 +311,20 @@ public void shouldMaterializeCorrectly() {
    // When:
    final PersistentQueryMetadata queryMetadata = queryBuilder.buildQuery(
        STATEMENT_TEXT,
-        QUERY_ID,
+        PULL_QUERY_ID,


This should be QUERY_ID as this is the id of the query that is building the materialized state, not the id of the pull query accessing it.

I believe that this should be actually the pull query ID because we do queryMetadata.getMaterialization() which is only called by pull queries.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/StaticQueryExecutor.java

ksql-engine/src/main/java/io/confluent/ksql/util/PersistentQueryMetadata.java

vpapavas · 2019-11-07T04:00:58Z

Are you up for giving that a go? Maybe leave this PR as is and have a go at creating a new one with the above approach. Then if we're stuck for time we can look to put this one if for the initial launch and then switch to the better approach later.

Yes I am! I will create a separate PR for it.

big-andy-coates

Thanks @vpapavas

Yes I am! I will create a separate PR for it.

Can you create a github issue in the pull query project to track this please, so that we don't forget, and reference it in this PR.

Can you also create another github issue to revert to a unique query id per request, (as this PR makes it a static id).

I've flagged up a couple of nits, (please fix), and also a couple of design issues. However, as noted below, I think we can live with the design issues as we have Github issues to track then and we quickly fix them post launch. I'm happy to help out fixing!

ksql-engine/src/main/java/io/confluent/ksql/util/PersistentQueryMetadata.java

big-andy-coates · 2019-11-14T10:59:16Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/StaticQueryExecutor.java

@@ -161,11 +162,10 @@ public static void validate(

      final WhereInfo whereInfo = extractWhereInfo(analysis, query);

-      final QueryId queryId = uniqueQueryId();
-      final QueryContext.Stacker contextStacker = new Stacker();
+      final QueryId queryId = new QueryId(QUERY_ID_PREFIX + getSourceName(analysis).name());


design note: this duplicates functionality in PersistentQueryMetadata that builds the query id. Such duplication is generally a poor design. If someone changes one bit of code without also changing the other, then the two get out of sync, and the query id returned to users will no longer reflect the query id logged to the processing log.

Hence, it is general good practice to avoid such duplication of code. Instead, define the code in one place and call it from the other, e.g. in this instance I would probably define QUERY_ID_PREFIX in this class, (as its specific to pull queries), not in PersistentQueryMetadata, which deals with all query types. I'd then add a method to build the query id from the prefix and the sink name and would then call this new method from here and from the PersistentQueryMetadata constructor.

However, in this case this is hopefully just a temporary fix to get ksqlDB over the line. As long as we have a Github issue to track this and we resolve it quickly after launch, then I'm happy for this to be merged as-is. Please reference the github issue here for future reference.

I agree this is a far better design. The reason I duplicated the code is that there is currently no dependency between the two modules, ksql-engine -> ksql-rest-app. Adding this dependency, creates a circular dependency between these modules. To address this, I added the method in PersistenQueryMetadata although I agree that it is more suited in StaticQueryExecutor.

Why do we even need a queryID here anymore (now that we're not passing it in)? it seems wrong to build it here at all to be honest... It's just being used in the TableRowsEntity and it looks like that queryID there is never being surfaced (cc @big-andy-coates you added it, what is it used for?). I think we should remove it

Also don't block your PR on this, it can be a follow-up

TableRowsEntity and it looks like that queryID there is never being surfaced (cc @big-andy-coates you added it, what is it used for?

Before we switch pull queries over to /query the TableRowsEntity was the entity returned to the caller - i.e. the caller got to see the query id, and each call got a unique query id. Hence users could use this query id when checking the processing log for any errors.

ksql-engine/src/main/java/io/confluent/ksql/util/PersistentQueryMetadata.java

removed extranneous synchronized and queryid fixed race condition Addressed Andy's comments

vpapavas requested a review from a team as a code owner October 31, 2019 00:10

vinothchandar approved these changes Oct 31, 2019

View reviewed changes

agavra previously requested changes Oct 31, 2019

View reviewed changes

agavra requested a review from a team October 31, 2019 17:04

vpapavas force-pushed the pull-transformations branch from 11be4a5 to 9f88966 Compare November 1, 2019 18:49

agavra reviewed Nov 4, 2019

View reviewed changes

agavra requested a review from a team November 4, 2019 16:59

vpapavas requested a review from big-andy-coates November 4, 2019 18:51

big-andy-coates suggested changes Nov 6, 2019

View reviewed changes

vpapavas changed the title ~~feat: cache transformations in PersistentQueryMetadata~~ refactor: cache transformations in PersistentQueryMetadata Nov 6, 2019

vpapavas force-pushed the pull-transformations branch from 2b98680 to d21e720 Compare November 7, 2019 23:53

big-andy-coates approved these changes Nov 14, 2019

View reviewed changes

This was referenced Nov 14, 2019

Pull queries: Create unique query ID in StaticQueryExecutor #3854

Closed

Pull queries: Build transformation when building CTAS query #3855

Closed

cache transformations between requests

b0ac339

removed extranneous synchronized and queryid fixed race condition Addressed Andy's comments

vpapavas force-pushed the pull-transformations branch from d21e720 to b0ac339 Compare November 14, 2019 22:32

vpapavas added 2 commits November 14, 2019 15:11

nits

5031b46

nit

b0a717f

vpapavas merged commit 537d23d into confluentinc:master Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: cache transformations in PersistentQueryMetadata #3708

refactor: cache transformations in PersistentQueryMetadata #3708

vpapavas commented Oct 31, 2019 •

edited

Loading

ghost commented Oct 31, 2019

vinothchandar left a comment

agavra Oct 31, 2019

vpapavas commented Nov 1, 2019

vinothchandar commented Nov 1, 2019

rodesai commented Nov 1, 2019

vpapavas commented Nov 1, 2019

vpapavas commented Nov 4, 2019

big-andy-coates left a comment

big-andy-coates Nov 6, 2019

vpapavas Nov 9, 2019

vpapavas commented Nov 7, 2019

big-andy-coates left a comment

big-andy-coates Nov 14, 2019

vpapavas Nov 14, 2019 •

edited

Loading

agavra Nov 14, 2019 •

edited

Loading

agavra Nov 14, 2019

big-andy-coates Nov 18, 2019

refactor: cache transformations in PersistentQueryMetadata #3708

refactor: cache transformations in PersistentQueryMetadata #3708

Conversation

vpapavas commented Oct 31, 2019 • edited Loading

Description

Testing done

Reviewer checklist

ghost commented Oct 31, 2019

vinothchandar left a comment

Choose a reason for hiding this comment

agavra Oct 31, 2019

Choose a reason for hiding this comment

vpapavas commented Nov 1, 2019

vinothchandar commented Nov 1, 2019

rodesai commented Nov 1, 2019

vpapavas commented Nov 1, 2019

vpapavas commented Nov 4, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Nov 6, 2019

Choose a reason for hiding this comment

vpapavas Nov 9, 2019

Choose a reason for hiding this comment

vpapavas commented Nov 7, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Nov 14, 2019

Choose a reason for hiding this comment

vpapavas Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

agavra Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

agavra Nov 14, 2019

Choose a reason for hiding this comment

big-andy-coates Nov 18, 2019

Choose a reason for hiding this comment

vpapavas commented Oct 31, 2019 •

edited

Loading

vpapavas Nov 14, 2019 •

edited

Loading

agavra Nov 14, 2019 •

edited

Loading