Added self documentation to the KSQL config public config variables. #422

hjafarpour · 2017-10-27T18:28:54Z

No description provided.

hjafarpour · 2017-10-27T18:51:18Z

retest this please

dguy · 2017-10-30T11:03:51Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.SHORT,
+            defaultSinkNumberOfReplications,
+            ConfigDef.Importance.MEDIUM,
+            "The default number of replications for the topics created by KSQL."


replications -> replicas

dguy · 2017-10-30T11:05:01Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.STRING,
+            KSQL_PERSISTENT_QUERY_NAME_PREFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Second part of the prefix for persitent queries.")


Perhaps a description of what the final query name would look like?

Added an example.

dguy · 2017-10-30T11:05:16Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.STRING,
+            KSQL_TRANSIENT_QUERY_NAME_PREFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Second part of the prefix for transient queries.")


Added an example.

dguy · 2017-10-30T11:06:16Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java


-  public int defaultSinkNumberOfPartitions = 4;
-  public short defaultSinkNumberOfReplications = 1;
+  public static int defaultSinkNumberOfPartitions = 4;


Do these need to be public, i.e., they are set as the defaults anyway

Not necessarily, changed them to private.

dguy · 2017-10-30T11:12:02Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            KSQL_TABLE_STATESTORE_NAME_SUFFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Suffix for state store names in Tables.")
+    .define(DEFAULT_SINK_NUMBER_OF_PARTITIONS,


I'm not sure why we have DEFAULT_SOME_PROPERTY and SOME_PROPERTY? We just define what the default value is here and then we can use SOME_PROPERTY everywhere. The ConfigDef takes care of providing the default value if none is supplied. So further down from line 141 etc you don't need to create a new copy of the Map and put the defaults in, they will already be there.

The idea is to let user define the default for a set of statements. For instance if user want to have the default value of partitions to be 10 for all of his/her queries they can set the default and just write their queries instead of having to specify it in each of their queries. On the other hand, if user wants to have a different value for a specific query he/she can set the value in the specific query syntax and it will be applied to only that one. User can change the default to another value and that value will be used as default partition number for all the queries after that.

dguy

@hjafarpour in the KsqlConfig ctor you shouldn't need to create a map for the ksqlConfigProps and add the defaults. That will already be done for you by the ConfigDef. The only map we need to create is the one for ksqStreamConfigProps and for that you should still be able to pull the defaults out of the ConfigDef

…umentation

hjafarpour · 2017-10-31T18:36:15Z

@dguy the properties in ksqlConfigProps and ksqlStreamConfigProps are not final and can be changed so in addition to the ConfigDef I have these two maps so we can have the latest values.

dguy · 2017-11-01T11:07:52Z

@hjafarpour sure, but you can still populate the maps from the defaults provided by the config, i.e, you can use super.originals() to get a map that has the user provided configs and/or defaults, so you can remove most of the code that is populating the ksqlConfigProps with default values.

…props.

hjafarpour · 2017-11-01T19:59:40Z

@dguy changed the init for the configs based on your feedback.

dguy · 2017-11-02T11:32:17Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java


    ksqlStreamConfigProps.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, defaultAutoOffsetRestConfig);
    ksqlStreamConfigProps.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, defaultCommitIntervalMsConfig);
    ksqlStreamConfigProps.put(
        StreamsConfig.CACHE_MAX_BYTES_BUFFERING_CONFIG, defaultCacheMaxBytesBufferingConfig);
    ksqlStreamConfigProps.put(StreamsConfig.NUM_STREAM_THREADS_CONFIG, defaultNumberOfStreamsThreads);

-    for (Map.Entry<?, ?> entry : props.entrySet()) {
+    for (Map.Entry<?, ?> entry : originals().entrySet()) {
      final String key = entry.getKey().toString();
      if (key.toLowerCase().startsWith(KSQL_CONFIG_PREPERTY_PREFIX)) {


i think we'd already have all the properties in the ksqlConfigProps by virtue of constructing the map from super.values(), so we could skip updating the ksqlConfigProps and only update ksqlStreamConfigProps if the key doesn't start with KSQL_CONFIG_PROPERTY_PREFIX

dguy

just one more comment, otherwise LGTM

…umentation

…via values. Correct the CLI test to expect the correct properties.

hjafarpour · 2017-11-02T21:05:18Z

@dguy made the change.

apurvam · 2017-11-02T23:48:34Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

-  public static final String SINK_NUMBER_OF_REPLICATIONS = "REPLICATIONS";
-  public static final String SINK_NUMBER_OF_REPLICATIONS_PROPERTY = "ksql.sink.replications";
-  public static final String DEFAULT_SINK_NUMBER_OF_REPLICATIONS = "ksql.sink.replications.default";
+  public static final String SINK_NUMBER_OF_REPLICAS = "REPLICAS";


The name and the value of this property don't seem to line up. What does the REPLICAS value mean exactly? Would it be more appropriate to have a numeric value? This applies to the SINK_NUMBER_OF_PARTITIONS property above as well. Also where is this SINK_NUMBER_OF_REPLICAS string used?

These are not part of the config variables that can be set by the user via the KsqlConfig but they will be set in the query to specify the number of partitions, replications and the timestamp column for the result stream/table. This is just a constant to represent the name of the attributes.
Here is an example:
CREATE STREAM enrichedpv1 with (timestamp='pageid', partitions = 3) AS SELECT users.userid AS userid, pageid as pageid, region, gender FROM pageview LEFT JOIN users ON pageview.userid = users.userid;

Hmm. Then why is it in KsqlConfig? There is also a SINK_NUMBER_OF_REPLICAS_PROPERTY which I assume does go in the ConfigDef? It would be better to have the literals that are in the query moved to a separate file.

Good idea, I'll move them to a new file, KsqlConstants.

apurvam · 2017-11-02T23:56:46Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.STRING,
+            KSQL_PERSISTENT_QUERY_NAME_PREFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Second part of the prefix for persitent queries. For instance if the prefix is transient_"


Did you mean transient_ here? IT doesn't fit in with the rest of the description.

It's the second part. The first part come from KSQL_SERVICE_ID_CONFIG(ksql.service.id)

Ok. I am still not sure I follow. I think it would make sense for the example to include the full chain, so that it is clearer to the user how it all comes together at each stage.

Added more details with an example.

The doc string here as written is:

Second part of the prefix for persitent queries. For instance if the prefix is transient_query_ the query name will be ksql_query_1.

Did you really mean for the example prefix to be 'transient_query_'?

apurvam · 2017-11-03T00:13:16Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.LONG,
+            defaultSinkWindowChangeLogAdditionalRetention,
+            ConfigDef.Importance.MEDIUM,
+            "The default window change log additional retention time."


You probably want to drop the 'default' here. Also, it may make sense to expand the description. It doesn't make sense as is. Perhaps a sentence like 'The amount of time to retain window change logs'. Actually I am still not sure what that means Also, there should be a time unit in the name of the config and the description.

Good point, added the time unit and more details to the description.

apurvam · 2017-11-03T20:04:42Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            "The default window change log additional retention time. This is a streams "
+            + "config value which will be added to a windows maintainMs to ensure data is not "
+            + "deleted from "
+            + "the "


Why these new lines?

ie. why have this string over so many lines?

Fixed it, seems that my intelliJ formatted it that way automatically.

…umentation

…ames.

apurvam

Thanks for the updates. It looks good to me, barring some doc strings which I think have some copy/paste errors.

apurvam · 2017-11-07T05:48:54Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.STRING,
+            KSQL_PERSISTENT_QUERY_NAME_PREFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Second part of the prefix for persitent queries. For instance if the prefix is transient_"


The doc string here as written is:

Second part of the prefix for persitent queries. For instance if the prefix is transient_query_ the query name will be ksql_query_1.

Did you really mean for the example prefix to be 'transient_query_'?

apurvam · 2017-11-07T05:49:27Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Type.STRING,
+            KSQL_TRANSIENT_QUERY_NAME_PREFIX_DEFAULT,
+            ConfigDef.Importance.MEDIUM,
+            "Second part of the prefix for transient queries. For instance if the prefix is "


Thanks for updating the example. It makes sense now!

apurvam · 2017-11-07T05:54:07Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

+            ConfigDef.Importance.MEDIUM,
+            "Suffix for state store names in Tables. For instance if the suffix is _ksql_statestore the state "
+            + "store name would be ksql_query_1_ksql_statestore"
+            + "_ksql_statestore ")


Did you add this extra line by mistake?

Good catch for 'transient_query_', it should be 'query_'. Fixed it.
IntelliJ added this, fixed it :)

apurvam

LGTM, thanks!

dguy

LGTM

Added self documentation to the KSQL config publis config variables.

db38492

hjafarpour requested review from dguy and apurvam October 27, 2017 18:29

dguy suggested changes Oct 30, 2017

View reviewed changes

miguno changed the title ~~Added self documentation to the KSQL config publis config variables.~~ Added self documentation to the KSQL config public config variables. Oct 30, 2017

Added examples to the descriptions.

17de98e

dguy reviewed Oct 31, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/4.0.x' into KSQLConfig-selfdoc…

34f2231

…umentation

Renamed some KSQL config props and used ConfigDef for populating the …

b155a65

…props.

dguy reviewed Nov 2, 2017

View reviewed changes

hjafarpour added 2 commits November 2, 2017 11:23

Merge remote-tracking branch 'upstream/4.0.x' into KSQLConfig-selfdoc…

94cc2db

…umentation

Only update ksqlStreamConfigProps since ksqlConfigProps is populated …

02a2f32

…via values. Correct the CLI test to expect the correct properties.

apurvam reviewed Nov 3, 2017

View reviewed changes

More details on a config value doc.

b3438c6

apurvam reviewed Nov 3, 2017

View reviewed changes

hjafarpour added 2 commits November 6, 2017 11:28

Merge remote-tracking branch 'upstream/4.0.x' into KSQLConfig-selfdoc…

5104828

…umentation

Created a new KsqlConstants file to hold constants such as property n…

7026f77

…ames.

apurvam reviewed Nov 7, 2017

View reviewed changes

Minor config doc fix.

ae5689a

apurvam approved these changes Nov 7, 2017

View reviewed changes

dguy approved these changes Nov 8, 2017

View reviewed changes

hjafarpour merged commit 6ae07fa into confluentinc:4.0.x Nov 8, 2017

Added self documentation to the KSQL config public config variables. #422

Added self documentation to the KSQL config public config variables. #422

Conversation

hjafarpour commented Oct 27, 2017

hjafarpour commented Oct 27, 2017

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

dguy left a comment

Choose a reason for hiding this comment

hjafarpour commented Oct 31, 2017

dguy commented Nov 1, 2017

hjafarpour commented Nov 1, 2017

Choose a reason for hiding this comment

dguy left a comment

Choose a reason for hiding this comment

hjafarpour commented Nov 2, 2017

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

apurvam Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apurvam left a 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

apurvam left a comment

Choose a reason for hiding this comment

dguy left a comment

Choose a reason for hiding this comment

apurvam Nov 3, 2017 •

edited

Loading