Enable Partition metric collection during ongoing execution to another k… #1538

vmaheshw · 2021-04-28T21:52:34Z

This PR resolves #1497.

…afka store

efeg

Thanks for the PR! Left some comments.

Overall direction seems reasonable (i.e. having an additional sample store to keep partition metric samples during the ongoing execution). But there are issues around clarity and actual implementation logic -- unless I am missing something, the KafkaWriteOnlySampleStore.java does not seem to be doing what it is supposed to do (i.e. store samples given to it in a new topic).

Given that this is a draft PR. I suspect the implementation of that class is in progress?

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/constants/MonitorConfig.java

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/MetricSampler.java

...c/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/KafkaWriteOnlySampleStore.java

efeg

Thanks for the updates!
Left some comments.

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/constants/MonitorConfig.java

...din/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleDuringExecutionStore.java

efeg · 2021-05-07T23:57:15Z

...din/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleDuringExecutionStore.java

+      short numberOfBrokersInCluster;
+      try {
+        numberOfBrokersInCluster = (short) adminClient.describeCluster().nodes().get(CLIENT_REQUEST_TIMEOUT_MS,
+            TimeUnit.MILLISECONDS).size();


Bad indentation -- please import the project style file in IntelliJ that was shared in an earlier feedback. After the import, if you cut and paste the code changes, it will update the indentations to use the expected format.

...din/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleDuringExecutionStore.java

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/MetricFetcher.java

efeg

Thanks for the updates -- left more comments.

efeg · 2021-05-10T22:02:57Z

...ontrol/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/KafkaSampleStore.java

  protected static final Duration CONSUMER_CLOSE_TIMEOUT = Duration.ofSeconds(10);
  // Keep additional windows in case some of the windows do not have enough samples.
  protected static final int ADDITIONAL_WINDOW_TO_RETAIN_FACTOR = 2;
  protected static final ConsumerRecords<byte[], byte[]> SHUTDOWN_RECORDS = new ConsumerRecords<>(Collections.emptyMap());
  protected static final Duration SAMPLE_POLL_TIMEOUT = Duration.ofMillis(1000L);

  protected static final int DEFAULT_NUM_SAMPLE_LOADING_THREADS = 8;
-  protected static final short DEFAULT_SAMPLE_STORE_TOPIC_REPLICATION_FACTOR = 2;
  protected static final int DEFAULT_PARTITION_SAMPLE_STORE_TOPIC_PARTITION_COUNT = 32;


Shouldn't protected static final int DEFAULT_PARTITION_SAMPLE_STORE_TOPIC_PARTITION_COUNT = 32; be removed from this class? -- i.e. it is in the abstract class already.

efeg · 2021-05-10T22:03:58Z

...rc/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/AbstractKafkaSampleStore.java

+  protected static final Duration PRODUCER_CLOSE_TIMEOUT = Duration.ofMinutes(3);
+  protected static final short DEFAULT_SAMPLE_STORE_TOPIC_REPLICATION_FACTOR = 2;
+  protected static final int DEFAULT_PARTITION_SAMPLE_STORE_TOPIC_PARTITION_COUNT = 32;
+  protected static final long DEFAULT_PARTITION_SAMPLE_STORE_TOPIC_RETENTION_TIME_MS = TimeUnit.HOURS.toMillis(1);


This seems only relevant to KafkaPartitionMetricSampleOngoingExecutionStore. Should we move it to that class?

efeg · 2021-05-10T22:55:04Z

...in/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleOngoingExecutionStore.java

+  }
+
+  @SuppressWarnings("unchecked")
+  protected void ensureTopicsCreated(Map<String, ?> config) {


Doesn't this function create 1 topic? Should we name it ensureTopicCreated?

...in/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleOngoingExecutionStore.java

...rc/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/ReadOnlyKafkaSampleStore.java

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/constants/MonitorConfig.java

efeg

Thanks for all the updates!
Left 2 more nits otherwise LGTM.

...rc/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/ReadOnlyKafkaSampleStore.java

...inkedin/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleOnExecutionStore.java

efeg

LGTM -- thanks for the PR!

…ng execution (linkedin#1538)

Enable Partition metric collection during task execution in another k…

0028147

…afka store

vmaheshw marked this pull request as draft April 28, 2021 21:52

efeg self-requested a review April 28, 2021 21:54

efeg requested changes May 4, 2021

View reviewed changes

cleanup the code

ca40f85

vmaheshw requested a review from efeg May 5, 2021 07:25

vmaheshw marked this pull request as ready for review May 7, 2021 17:29

efeg reviewed May 8, 2021

View reviewed changes

Vaibhav Maheshwari added 4 commits May 10, 2021 12:21

address comments

b5d8d90

address comments

6b97778

commit conflicts

fdd28e9

refactor the config names

a9ccbbb

vmaheshw changed the title ~~Enable Partition metric collection during task execution in another k…~~ Enable Partition metric collection during ongoing execution in another k… May 10, 2021

vmaheshw changed the title ~~Enable Partition metric collection during ongoing execution in another k…~~ Enable Partition metric collection during ongoing execution to another k… May 10, 2021

vmaheshw requested a review from efeg May 10, 2021 20:18

efeg reviewed May 10, 2021

View reviewed changes

Vaibhav Maheshwari added 2 commits May 10, 2021 17:10

address comments

a3089f5

address comments

6151e43

vmaheshw requested a review from efeg May 11, 2021 00:12

efeg reviewed May 11, 2021

View reviewed changes

...rc/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/ReadOnlyKafkaSampleStore.java Outdated Show resolved Hide resolved

...inkedin/kafka/cruisecontrol/monitor/sampling/KafkaPartitionMetricSampleOnExecutionStore.java Outdated Show resolved Hide resolved

address comments

2f24d03

vmaheshw requested a review from efeg May 11, 2021 00:29

efeg approved these changes May 11, 2021

View reviewed changes

efeg merged commit 546ab51 into linkedin:master May 11, 2021

efeg pushed a commit to efeg/cruise-control that referenced this pull request May 11, 2021

Enable partition metric collection to a configured topic during ongoi…

82c7fcb

…ng execution (linkedin#1538)

efeg pushed a commit to efeg/cruise-control that referenced this pull request May 11, 2021

Enable partition metric collection to a configured topic during ongoi…

85fe566

…ng execution (linkedin#1538)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Partition metric collection during ongoing execution to another k… #1538

Enable Partition metric collection during ongoing execution to another k… #1538

vmaheshw commented Apr 28, 2021 •

edited

Loading

efeg left a comment

efeg left a comment

efeg May 7, 2021

efeg left a comment

efeg May 10, 2021

efeg May 10, 2021

efeg May 10, 2021

efeg left a comment

efeg left a comment

Enable Partition metric collection during ongoing execution to another k… #1538

Enable Partition metric collection during ongoing execution to another k… #1538

Conversation

vmaheshw commented Apr 28, 2021 • edited Loading

efeg left a comment

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg May 7, 2021

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg May 10, 2021

Choose a reason for hiding this comment

efeg May 10, 2021

Choose a reason for hiding this comment

efeg May 10, 2021

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

efeg left a comment

Choose a reason for hiding this comment

vmaheshw commented Apr 28, 2021 •

edited

Loading