tomasfabian / ksqlDB.RestApi.Client-DotNet

ksqlDb.RestApi.Client is a C# LINQ-enabled client API for issuing and consuming ksqlDB push and pull queries and executing statements.
MIT License
93 stars 24 forks source link

CreateQueryStream doesn't always use configured parameters #75

Closed jbkuczma closed 2 months ago

jbkuczma commented 2 months ago

I've observed that CreateQueryStream does not always use the configured query stream parameters. I have a web app that will make a graphql query request that uses CreatePullQuery<ModelOne>("MATERIALIZED_VIEW_ONE"). When the query succeeds the web app will then open a subscription that uses CreateQueryStream<ModelOne>("MATERIALIZED_VIEW_ONE").

Setup and dependency injection

services.AddDbContext<IMyKSqlDbContext, MyKSqlDbContext>(options =>
{
    var builder = options
        .UseKSqlDb("endpoint")
        .SetBasicAuthCredentials("apiKey", "apiSecret")
        .SetupQueryStream(opt =>
        {
            opt.AutoOffsetReset = AutoOffsetReset.Latest;
            opt.Properties[KSqlDbConfigs.KsqlQueryPushV2Enabled] = "true";
            opt.Properties[KSqlDbConfigs.KsqlQueryPushV2ContinuationTokensEnabled] = "true";
        });

    builder.Options.ShouldPluralizeFromItemName = false;
    builder.Options.DisposeHttpClient = true;
    builder.Options.IdentifierEscaping = IdentifierEscaping.Always;
}, ServiceLifetime.Transient, restApiLifetime: ServiceLifetime.Transient);

Console output when calling the query that uses CreatePullQuery.

info: ksqlDb.RestApi.Client[0]
Executing query Sql: SELECT * FROM `MATERIALIZED_VIEW_ONE`
      WHERE (`ModelId` = 'some_id') AND (`Date` = '2024-04-23');
      Parameters:
      ksql.streams.auto.offset.reset = earliest

Console output when calling the subscription that uses CreateQueryStream

info: ksqlDb.RestApi.Client[0]
      Executing query Sql: SELECT * FROM `MATERIALIZED_VIEW_ONE`
      WHERE (`ModelId` = 'some_id') AND (`Date` = '2024-04-23') EMIT CHANGES;
      Parameters:
      ksql.streams.auto.offset.reset = earliest

I would expect CreateQueryStream to always use the configured properties set in the SetupQueryStream.

Attached is also a screenshot from my debugger of QueryParameters and QueryStreamParameters in KSqlDBContextOptions of the injected query context.

Screenshot 2024-04-23 at 11 57 46 AM
tomasfabian commented 2 months ago

Hello @jbkuczma, At present, the main disparity lies in the fact that CreatePullQuery continues to utilize the /query endpoint due to historical reasons, while CreateQueryStream leverages the /query-stream endpoint.

The deprecation of the /query endpoint was proposed as part of KLIP-15 in favor of the new HTTP/2-based /query-stream. However, the deprecation itself has not been scheduled yet.

Regarding the potential upgrade to utilize HTTP/2 for non-NETSTANDARD builds, it's crucial to recognize that such a change would constitute a breaking change, correct?

jbkuczma commented 2 months ago

Yes I agree that that sounds like it would be a breaking change though I don't think it should affect this issue. My issue is it appears that the parameters I have set to be used by CreateQueryStream were not used. I should be able to use the two endpoints alongside each other or is that thought incorrect?

tomasfabian commented 2 months ago

Your use case is indeed valid if I understand it correctly, @jbkuczma. Could you share the package version you're currently using?

tomasfabian commented 2 months ago

The problem arises from the fact that the PullQuery overrides the options set for the PushQuery due to its utilization of a different endpoint at the moment. To address this, I will introduce a new setting for configuring the endpoint type:

optionsBuilder.SetEndpointType(EndpointType.Query);

By default, it will be set to EndpointType.QueryStream

/// <summary>
/// Specifies KSQL query endpoints when using the ksqlDB REST API.
/// </summary>
public enum EndpointType
{
  /// <summary>
  /// Represents the "/query" endpoint in the ksqlDB REST API.
  /// </summary>
  Query,

  /// <summary>
  /// Represents the "/query-stream" endpoint in the ksqlDB REST API.
  /// </summary>
  QueryStream
}

Do you believe that in a future release, it would be beneficial to separate the configuration for push and pull queries?

options
  .SetupPullQuery(opt =>
  {
  }
  .SetupQueryStream(opt =>
  {
  });

As a temporary solution, until I implement a fix, you can attempt to configure the options using the SetupQuery as follows:

options.SetupQuery(opt =>
{
    opt.Properties[QueryStreamParameters.AutoOffsetResetPropertyName] = AutoOffsetReset.Latest.ToKSqlValue();
    opt.Properties[KSqlDbConfigs.KsqlQueryPushV2Enabled] = "true";
    opt.Properties[KSqlDbConfigs.KsqlQueryPushV2ContinuationTokensEnabled] = "true";
});

Sorry for the inconvenience.

jbkuczma commented 2 months ago

@tomasfabian I am using version 5.0.0. I think it could be useful to provide the option of configuring pull queries.

tomasfabian commented 2 months ago

Could you please review this PR #76 where I merged KSqlDBContext's CreateStream and CreateQueryStream? I also added the mentioned SetEndpointType. You can also try out the release candidate package.

dotnet add package ksqlDb.RestApi.Client --version 6.0.0-rc.1
jbkuczma commented 2 months ago

Briefly tested out the new version and from my usage it's looking good. I can look over the PR tomorrow.

jbkuczma commented 2 months ago

@tomasfabian I looked over your PR and nothing jumped out to me.

tomasfabian commented 2 months ago

Thank you very much @jbkuczma! I created a follow-up issue for separating the pull query parameters: https://github.com/tomasfabian/ksqlDB.RestApi.Client-DotNet/issues/77