Describe extend functionality #511

bluemonk3y · 2017-12-07T00:23:10Z

#470 - to include useful information. Functionality keeps the existing 'DESCRIBE' function, which is then enhanced via 'DESCRIBE EXTENDED <stream, table, query>' to show metrics, topic, key-field etc

dguy

Overall, LGTM. Just wondering if we added any tests for the describe extended? Didn't see any jump out at me

bluemonk3y · 2017-12-07T22:58:07Z

@dguy - no tests were added for 'describe extend' functionality, it's not really doing much apart from pulling existing information and munging it into a pojo; lots of plumbing.

I guess I could put one in the CLI.test - as the extend functionality just enriches a POJO that is passed back to the client and printed to stdout. - what do you thunk?

…lity

apurvam

LGTM, barring one comment.

apurvam · 2017-12-09T00:48:11Z

ksql-common/src/main/java/io/confluent/ksql/errors/LogMetricAndContinueExceptionHandler.java

+            exception);
+
+    MetricCollectors.recordError(record.topic());
+    return DeserializationHandlerResponse.CONTINUE;


Shouldn't this return value depend on how FAIL_ON_DESERIALIZATION is configured? Do we want to continue if FAIL_ON_DESERIALIZATION is false? Presumably the query is failed elsewhere in the latter case. If so, a comment here would help.

Thanks Apuva - if we fail then all processing stops; this impl is identical to the streams one. This is the side-channel where the user has the option to provide their own handler - however, a bad data format instance doesn't kill the stream processor.

hjafarpour · 2017-12-11T22:18:48Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/entity/SourceDescription.java

  private final  List<FieldSchemaInfo> schema;
-  private final DataSource.DataSourceType type;
+  private final String type;


Why do we have to change this to string?

@hjafarpour because it was returning a Query which is not a valid value on the enum

hjafarpour · 2017-12-11T22:20:59Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

-import io.confluent.ksql.parser.tree.ShowColumns;
-import io.confluent.ksql.parser.tree.Statement;
-import io.confluent.ksql.parser.tree.TerminateQuery;
+import io.confluent.ksql.parser.tree.*;


I think if our more strict code style will require not using *.

Good catch. We should not be using wild card imports here or elsewhere. I am surprised that this passes the stylecheck.

hjafarpour · 2017-12-11T22:43:20Z

I still think we should separate Stream/Table, which represent data sources/sinks, from Queries which represent computation.
If @miguno is ok with this iteration from product point of view for this PR, let's merge it but let's keep track if my suggestions for a future PR which can include the following:

Describe Extended [stream/table name];
This will present the detailed information on the stream or table. We wouldn't show any query informations but we would show all the queries that read from/write into this stream/table. Note that we assume N-to-N relationship between queries and streams/tables so we will have set of queries associated to this stream/table. We can show the id of these queries in the result of this statement too.
Describe Query [query id];
This will show all the details on a query including the execution plan, streams topology and producer/consumer stats. Note that producer/consumer stats are for queries, aka streams jobs and not for data sources/sinks, aka streams/tables.

Finally, we would indicate that the shown data is only for one server/instance and in order to get the wholistic view of the metrics user should gather the metrics from all instances of a query.

apurvam · 2017-12-12T01:54:41Z

There seems to be a bug with the ConsumerCollector: the ConsumerCollector.onConsume method takes an argument of type ConsumerRecords<String, List<ConsumerRecord<K,V>>.

The collector counts every invocatino of onConsume as a single event. So even if 10000 records were consumed (and passed into the onConsume method), it would only count as 1, causing the metric to be wrong.

apurvam · 2017-12-12T02:00:01Z

Sorry, I misread the code. The current consumer collector behavior seems correct.

bluemonk3y · 2017-12-12T08:54:24Z

@hjafarpour - as discussed I will document this on the wiki and sync with @miguno

dguy

LGTM after you have merged

bluemonk3y · 2017-12-12T11:18:33Z

retest this please

bluemonk3y · 2017-12-12T12:46:39Z

retest this please

bluemonk3y added 18 commits November 21, 2017 18:36

Adding first cut of metric-collection

4f48d65

updates to extract Counter at the partition level

9635688

minor checkstyle changes

e2ecf68

Merge remote-tracking branch 'upstream/master' into SimpleMetricHooks

3f90905

hooking into the front end - cannot do aggs atm

03a99b6

Last cut before switching to Kafka metrics

d31f803

switch over to kafka metrics

94bc6d8

minor code analysis changes

89ab590

forgot to remove counted

3b4ac4a

final refactor - split apart producer consumer stuff

835d62a

final rework for dguys feedback

d4f2cb3

final PR items for dguy

321664f

cli log error msg change

620697b

updates from apurva comments

ad78ac0

Merge remote-tracking branch 'upstream/master' into SimpleMetricHooks

36e9976

Merge remote-tracking branch 'upstream/master' into SimpleMetricHooks

4a009e3

adding DESCRIBE EXTENDED functionality

8219bb6

Merge remote-tracking branch 'upstream/master' into DescribeExtend

d5bccec

bluemonk3y requested review from dguy, apurvam and hjafarpour December 7, 2017 00:23

dguy reviewed Dec 7, 2017

View reviewed changes

bluemonk3y added 3 commits December 7, 2017 15:04

added support for error-tracking, also cleanedup extend-cli functiona…

b6d33e1

…lity

Merge remote-tracking branch 'upstream/master' into DescribeExtend

24d0593

more output enhancement

bdd6f46

apurvam approved these changes Dec 9, 2017

View reviewed changes

hjafarpour reviewed Dec 11, 2017

View reviewed changes

hjafarpour mentioned this pull request Dec 12, 2017

View Tables Properties for a Stream #516

Closed

dguy approved these changes Dec 12, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into DescribeExtend

40b07c1

orgd imports

b7114ea

bluemonk3y merged commit 6ca6220 into confluentinc:master Dec 12, 2017

bluemonk3y mentioned this pull request Dec 12, 2017

Support DESCRIBE EXTENDED, v2 #522

Merged

bluemonk3y deleted the DescribeExtend branch January 5, 2018 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Describe extend functionality #511

Describe extend functionality #511

bluemonk3y commented Dec 7, 2017

dguy left a comment

bluemonk3y commented Dec 7, 2017

apurvam left a comment

apurvam Dec 9, 2017

bluemonk3y Dec 9, 2017

hjafarpour Dec 11, 2017

bluemonk3y Dec 12, 2017

hjafarpour Dec 11, 2017

apurvam Dec 12, 2017

bluemonk3y Dec 12, 2017

hjafarpour commented Dec 11, 2017

apurvam commented Dec 12, 2017

apurvam commented Dec 12, 2017

bluemonk3y commented Dec 12, 2017 •

edited

Loading

dguy left a comment

bluemonk3y commented Dec 12, 2017

bluemonk3y commented Dec 12, 2017

Describe extend functionality #511

Describe extend functionality #511

Conversation

bluemonk3y commented Dec 7, 2017

dguy left a comment

Choose a reason for hiding this comment

bluemonk3y commented Dec 7, 2017

apurvam left a comment

Choose a reason for hiding this comment

apurvam Dec 9, 2017

Choose a reason for hiding this comment

bluemonk3y Dec 9, 2017

Choose a reason for hiding this comment

hjafarpour Dec 11, 2017

Choose a reason for hiding this comment

bluemonk3y Dec 12, 2017

Choose a reason for hiding this comment

hjafarpour Dec 11, 2017

Choose a reason for hiding this comment

apurvam Dec 12, 2017

Choose a reason for hiding this comment

bluemonk3y Dec 12, 2017

Choose a reason for hiding this comment

hjafarpour commented Dec 11, 2017

apurvam commented Dec 12, 2017

apurvam commented Dec 12, 2017

bluemonk3y commented Dec 12, 2017 • edited Loading

dguy left a comment

Choose a reason for hiding this comment

bluemonk3y commented Dec 12, 2017

bluemonk3y commented Dec 12, 2017

bluemonk3y commented Dec 12, 2017 •

edited

Loading