ListGroups and DescribeGroups protocol #770

wooyeong · 2017-09-20T02:32:23Z

wooyeong · 2017-09-20T02:35:13Z

Oh, I think correlationId and request object should go inside mapValuesSeries before this.queueCallback. I'll fix it.

hyperlink · 2017-09-20T13:43:02Z

lib/kafkaClient.js

+      // removing {error: null} before merging
+      results = _.map(results, result => _.omitBy(result, _.isNull));
+
+      callback(null, _.merge({}, ...(_.values(results))));


We are still supporting node 4 so spread can't be used here.

I'll change it to use .apply instead

hyperlink · 2017-09-20T13:50:59Z

lib/kafkaClient.js

+      this.queueCallback(broker.socket, correlationId, [
+        protocol.decodeListGroups,
+        cb,
+        this.options.versions.requestTimeout


this can be omitted if using the default requestTimeout value.

hyperlink · 2017-09-20T13:57:53Z

lib/kafkaClient.js

+    return callback(new Error('Client is not ready (getListGroups)'));
+  }
+  const brokers = this.brokerMetadata;
+  async.mapValuesSeries(brokers,


Series is not need here since there's no ordering requirement on the result correct? Lets we change this to mapValuesLimit instead with a default (configurable) limit of 10.

hyperlink · 2017-09-20T14:04:17Z

lib/kafkaClient.js

+  if (!this.ready) {
+    return callback(new Error('Client is not ready (getDescribeGroups)'));
+  }
+  const self = this;


Is setting self necessary since we're using fat arrows throughout?

hyperlink · 2017-09-20T14:21:17Z

lib/kafkaClient.js

+        return;
+      }
+
+      async.mapValuesSeries(results,


mapValuesLimit ?

hyperlink · 2017-09-20T14:27:22Z

lib/kafkaClient.js

+          this.queueCallback(broker.socket, correlationId, [
+            protocol.decodeDescribeGroups,
+            cb,
+            this.options.versions.requestTimeout


same comment as above this is optional.

hyperlink

Thanks for the PR. Any plans to write a test for this?

wooyeong · 2017-09-21T03:44:34Z

I've changed all mentioned problems, and introduced a new option options.maxAsyncRequests which defaults to 10 to feed mapValuesLimit.

For test, I think I can't write tests right now because I have at least minimum working code and have to make service first. I may try in the 1st week of October if it's not done.

hyperlink · 2017-09-21T17:29:24Z

lib/kafkaClient.js

+    (brokerMetadata, brokerId, cb) => {
+      const broker = this.brokerForLeader(brokerId);
+      if (!broker || !broker.isConnected()) {
+        return callback(new errors.BrokerNotAvailableError('Broker not available (getListGroups)'));


callback should be cb here.

hyperlink · 2017-09-21T17:31:44Z

lib/kafkaClient.js

+    return callback(new Error('Client is not ready (getDescribeGroups)'));
+  }
+
+  async.groupBy(groups,


Could we use groupByLimit here instead?

hyperlink · 2017-09-21T17:32:13Z

lib/kafkaClient.js

+        (groups, coordinator, cb) => {
+          const broker = this.brokerForLeader(coordinator);
+          if (!broker || !broker.isConnected()) {
+            return callback(new errors.BrokerNotAvailableError('Broker not available (getDescribeGroups)'));


this should be cb here and not that main callback.

hyperlink · 2017-09-21T17:32:53Z

lib/kafkaClient.js

+          broker.write(request);
+        },
+        (err, res) => {
+          callback(err || null, res);


could we flatten the result so it's keyed by groupId?

hyperlink

Looks much better thanks for the update!

Can you update the README with these new APIs ?

This is optional but you can also create a new Admin wrapper class if so inclined.

wooyeong · 2017-10-05T09:58:09Z

Changed as you requested. I also wrote new admin wrapper class and README.

For Admin, I reject if user provides older Client, but I don't know it's recommended way to do so.

hyperlink · 2017-10-05T13:36:23Z

Thanks for sticking with this @wooyeong ! Great feature 👍

wooyeong added 2 commits September 19, 2017 16:57

ListGroups and DescribeGroup protocol implemented

00e9d82

kafkaClient utilizes ListGroups and DescribeGroups

513844d

get correlationId just before making request

d14d8e7

wooyeong force-pushed the adminapi branch from 225403a to d14d8e7 Compare September 20, 2017 02:38

wooyeong added 2 commits September 20, 2017 11:45

fix wrong name used in error object

637eebd

adding missed space before function paren

d2d8957

hyperlink reviewed Sep 20, 2017

View reviewed changes

lib/kafkaClient.js Outdated

return;

}

async.mapValuesSeries(results,

Copy link

Collaborator

hyperlink Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapValuesLimit ?

hyperlink reviewed Sep 20, 2017

View reviewed changes

hyperlink requested changes Sep 20, 2017

View reviewed changes

fix reviewed problems

6142335

hyperlink reviewed Sep 21, 2017

View reviewed changes

hyperlink requested changes Sep 21, 2017

View reviewed changes

wooyeong force-pushed the adminapi branch from 0e01ddc to 6142335 Compare September 25, 2017 08:23

hyperlink mentioned this pull request Sep 27, 2017

Delete a topic (feature request) #237

Closed

wooyeong added 3 commits October 5, 2017 18:37

fix for review

7c950f0

introducing admin wrapper class for administrative apis

ff929f6

README entry for Admin wrapper class

20c7f97

hyperlink approved these changes Oct 5, 2017

View reviewed changes

hyperlink merged commit c1044c4 into SOHU-Co:master Oct 5, 2017

wooyeong deleted the adminapi branch October 5, 2017 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ListGroups and DescribeGroups protocol #770

ListGroups and DescribeGroups protocol #770

wooyeong commented Sep 20, 2017

wooyeong commented Sep 20, 2017

hyperlink Sep 20, 2017 •

edited

Loading

wooyeong Sep 21, 2017

hyperlink Sep 20, 2017

hyperlink Sep 20, 2017 •

edited

Loading

hyperlink Sep 20, 2017

hyperlink Sep 20, 2017

hyperlink Sep 20, 2017

hyperlink left a comment

wooyeong commented Sep 21, 2017

hyperlink Sep 21, 2017

hyperlink Sep 21, 2017

hyperlink Sep 21, 2017 •

edited

Loading

hyperlink Sep 21, 2017

hyperlink left a comment

wooyeong commented Oct 5, 2017

hyperlink commented Oct 5, 2017

ListGroups and DescribeGroups protocol #770

ListGroups and DescribeGroups protocol #770

Conversation

wooyeong commented Sep 20, 2017

wooyeong commented Sep 20, 2017

hyperlink Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperlink Sep 20, 2017 • edited Loading

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

hyperlink left a comment

Choose a reason for hiding this comment

wooyeong commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperlink Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperlink left a comment

Choose a reason for hiding this comment

wooyeong commented Oct 5, 2017

hyperlink commented Oct 5, 2017

hyperlink Sep 20, 2017 •

edited

Loading

hyperlink Sep 20, 2017 •

edited

Loading

hyperlink Sep 21, 2017 •

edited

Loading