Feature/zenko 266 ingestion consumer #332

JianqinWang · 2018-06-25T23:14:09Z

Reopening #261 with updated branch name format for Bert-e

bert-e · 2018-06-25T23:14:10Z

Hello JianqinWang,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get
information on this process.

Status report is not available.

bert-e · 2018-06-25T23:14:12Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

2 peers

ironman-machine · 2018-06-25T23:51:45Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-26T17:38:22Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-27T00:29:42Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-27T00:35:50Z

PR has been updated. Reviewers, please be cautious.

jonathan-gramain · 2018-06-27T18:38:55Z

lib/models/QueueEntry.js

@@ -1,9 +1,9 @@
 const { usersBucket } = require('arsenal').constants;

 const ObjectQueueEntry =
-    require('../../extensions/replication/utils/ObjectQueueEntry');
+    require('../../extensions/utils/ObjectQueueEntry');


Since we're moving those helper classes, let's move them to lib/models/ instead beside QueueEntry class.

jonathan-gramain · 2018-06-27T18:47:44Z

extensions/index.js

@@ -5,7 +5,7 @@ const extensions = {};

 fs.readdirSync(__dirname).forEach(moduleDir => {
    const extStat = fs.statSync(path.join(__dirname, moduleDir));
-    if (extStat.isDirectory()) {
+    if (extStat.isDirectory() && moduleDir !== 'utils') {


Note: if you move the helper classes to lib/models/ you should be able to get rid of this special case.

jonathan-gramain · 2018-06-27T18:49:41Z

extensions/mongoProcessor/MongoQueueProcessor.js

+        this._consumer = new BackbeatConsumer({
+            // zookeeper: { connectionString: this.zkConfig.connectionString },
+            topic: this.mongoProcessorConfig.topic,
+            groupId: `${this.mongoProcessorConfig.groupId}1`,


why suffixing with 1?

jonathan-gramain · 2018-06-27T18:51:59Z

extensions/mongoProcessor/MongoQueueProcessor.js

+            queueProcessor: this.processKafkaEntry.bind(this),
+        });
+        this._consumer.on('ready', () => {
+            this._consumer.subscribe();


Taylor recently added kubernetes healthcheck support, you would have to add the small support code around here to link to the healthcheck server (see changes in other backbeat components for examples, it should be almost a cut&paste).

jonathan-gramain · 2018-06-27T18:54:04Z

extensions/mongoProcessor/MongoQueueProcessor.js

+                this.logger, err => {
+                    if (err) {
+                        this.logger.error('error deleting object from mongo',
+                        { error: err });


{ bucket, key, error: err.message }

jonathan-gramain · 2018-06-27T19:00:54Z

extensions/mongoProcessor/MongoQueueProcessor.js

+                this.logger, err => {
+                    if (err) {
+                        this.logger.error('error putting to usersbucket',
+                        { error: err, bucketOwnerKey });


error: err.message

jonathan-gramain · 2018-06-27T19:03:45Z

extensions/mongoProcessor/MongoQueueProcessor.js

+                        { error: err, bucketOwnerKey });
+                        return done(err);
+                    }
+                    this.logger.info('successfully put to usersBucket',


usersBucket is internal and should not be exposed in an info log. What about bucket entry put into mongo? Not convinced entry is the best wording, let you decide :)

jonathan-gramain · 2018-06-27T19:06:18Z

extensions/mongoProcessor/MongoQueueProcessor.js

+        return process.nextTick(done);
+    }
+
+    /**


This is legacy code, please remove (now we can bootstrap by passing a bootstrap parameter to BackbeatConsumer class, but no need to deal with it if you don't use it in actual tests).

jonathan-gramain · 2018-06-27T19:07:37Z

extensions/replication/queueProcessor/QueueProcessor.js

@@ -11,14 +11,14 @@ const RoundRobin = require('arsenal').network.RoundRobin;
 const BackbeatProducer = require('../../../lib/BackbeatProducer');
 const BackbeatConsumer = require('../../../lib/BackbeatConsumer');
 const VaultClientCache = require('../../../lib/clients/VaultClientCache');
-const QueueEntry = require('../../../lib/models/QueueEntry');
+const QueueEntry = require('../../utils/QueueEntry');


Is there a good reason to move this file?

jonathan-gramain · 2018-06-27T19:21:43Z

extensions/utils/RaftLogEntry.js

+     *
+     * @param {object} objectMd - object info to format entry
+     * @param {string} bucketPrefix - prefix for bucketname to avoid name clash
+     * @return {object} JSON.stringified entry value to be sent to kafka


The description is a little bit off (or confusing), what is returned is a regular JS object that contains the stringified JSON kafka message as its value attribute.

ironman-machine · 2018-06-28T17:50:13Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-28T17:50:13Z

CONFLICT (add/add): Merge conflict in tests/unit/api/BackbeatRequest.spec.js
CONFLICT (add/add): Merge conflict in tests/unit/api/BackbeatAPI.spec.js
CONFLICT (add/add): Merge conflict in tests/functional/replication/queueProcessor.js
CONFLICT (add/add): Merge conflict in tests/functional/api/BackbeatServer.js
CONFLICT (add/add): Merge conflict in tests/functional/BackbeatConsumer.js
CONFLICT (add/add): Merge conflict in package.json
CONFLICT (add/add): Merge conflict in lib/models/QueueEntry.js
CONFLICT (add/add): Merge conflict in lib/management/index.js
CONFLICT (add/add): Merge conflict in lib/clients/backbeat-2017-07-01.api.json
CONFLICT (add/add): Merge conflict in lib/api/BackbeatRequest.js
CONFLICT (add/add): Merge conflict in lib/api/BackbeatAPI.js
CONFLICT (add/add): Merge conflict in lib/MetricsProducer.js
CONFLICT (add/add): Merge conflict in lib/BackbeatConsumer.js
CONFLICT (add/add): Merge conflict in extensions/replication/tasks/UpdateReplicationStatus.js
CONFLICT (add/add): Merge conflict in extensions/replication/tasks/ReplicateObject.js
CONFLICT (add/add): Merge conflict in extensions/replication/tasks/MultipleBackendTask.js
CONFLICT (add/add): Merge conflict in extensions/replication/replicationStatusProcessor/ReplicationStatusProcessor.js
CONFLICT (add/add): Merge conflict in extensions/replication/queueProcessor/task.js
CONFLICT (add/add): Merge conflict in extensions/replication/queueProcessor/QueueProcessor.js
CONFLICT (add/add): Merge conflict in extensions/replication/management.js
CONFLICT (add/add): Merge conflict in extensions/replication/failedCRR/FailedCRRProducer.js
CONFLICT (add/add): Merge conflict in extensions/replication/ReplicationQueuePopulator.js
CONFLICT (add/add): Merge conflict in extensions/lifecycle/management.js
CONFLICT (add/add): Merge conflict in extensions/lifecycle/conductor/LifecycleConductor.js
CONFLICT (add/add): Merge conflict in extensions/index.js
CONFLICT (add/add): Merge conflict in eve/main.yml
CONFLICT (add/add): Merge conflict in conf/config.json
CONFLICT (add/add): Merge conflict in conf/Config.js
CONFLICT (add/add): Merge conflict in bin/queuePopulator.js

bert-e · 2018-06-28T17:50:15Z

Conflict

There is a conflict between your branch feature/ZENKO-266-Ingestion-Consumer and the
destination branch development/8.0.

Please resolve the conflict on the feature branch (feature/ZENKO-266-Ingestion-Consumer).

#!bash
 $ git fetch
 $ git checkout origin/feature/ZENKO-266-Ingestion-Consumer
 $ git merge origin/development/8.0
 $ # <intense conflict resolution>
 $ git push origin HEAD:feature/ZENKO-266-Ingestion-Consumer

ironman-machine · 2018-06-28T17:53:21Z

PR has been updated. Reviewers, please be cautious.

bert-e · 2018-06-28T17:53:22Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

2 peers

ironman-machine · 2018-06-28T19:05:42Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-28T19:05:43Z

CONFLICT (add/add): Merge conflict in package.json
CONFLICT (add/add): Merge conflict in lib/models/QueueEntry.js
CONFLICT (add/add): Merge conflict in lib/MetricsProducer.js
CONFLICT (add/add): Merge conflict in lib/BackbeatConsumer.js
CONFLICT (add/add): Merge conflict in extensions/replication/queueProcessor/QueueProcessor.js
CONFLICT (add/add): Merge conflict in extensions/replication/ReplicationQueuePopulator.js
CONFLICT (add/add): Merge conflict in extensions/index.js
CONFLICT (add/add): Merge conflict in conf/config.json

ironman-machine · 2018-06-29T18:07:58Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T00:35:24Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T00:35:25Z

CONFLICT (add/add): Merge conflict in package.json
CONFLICT (add/add): Merge conflict in lib/models/QueueEntry.js
CONFLICT (add/add): Merge conflict in lib/MetricsProducer.js
CONFLICT (add/add): Merge conflict in lib/BackbeatConsumer.js
CONFLICT (add/add): Merge conflict in extensions/replication/queueProcessor/QueueProcessor.js
CONFLICT (add/add): Merge conflict in extensions/replication/ReplicationQueuePopulator.js
CONFLICT (add/add): Merge conflict in extensions/lifecycle/management.js
CONFLICT (add/add): Merge conflict in extensions/index.js
CONFLICT (add/add): Merge conflict in conf/config.json

ironman-machine · 2018-06-30T00:36:02Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T00:45:12Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T00:50:10Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T00:50:10Z

CONFLICT (add/add): Merge conflict in package.json
CONFLICT (add/add): Merge conflict in lib/models/RaftLogEntry.js
CONFLICT (add/add): Merge conflict in lib/models/QueueEntry.js
CONFLICT (add/add): Merge conflict in lib/MetricsProducer.js
CONFLICT (add/add): Merge conflict in lib/BackbeatConsumer.js
CONFLICT (add/add): Merge conflict in extensions/replication/queueProcessor/QueueProcessor.js
CONFLICT (add/add): Merge conflict in extensions/replication/ReplicationQueuePopulator.js
CONFLICT (add/add): Merge conflict in conf/config.json

ironman-machine · 2018-06-30T08:47:10Z

PR has been updated. Reviewers, please be cautious.

ironman-machine · 2018-06-30T09:06:03Z

PR has been updated. Reviewers, please be cautious.

philipyoo · 2018-06-30T22:03:50Z

extensions/mongoProcessor/MongoQueueProcessor.js

+     */
+    start() {
+        this.logger.info('starting mongo queue processor');
+        this._mongoClient.setup(err => {


i think with kube environment, and use of liveness probe, this is ok since the pod would just restart, but this could fail to start in other environments since we aren't waiting on each asynchronous setup of internal components

philipyoo · 2018-06-30T22:08:08Z

extensions/mongoProcessor/mongoProcessorTask.js

+    dump: config.log.dumpLevel });
+
+mongoQueueProcessor.start();
+healthServer.start();


this is missing setting onReadyCheck callback

jonathan-gramain · 2018-06-30T22:05:20Z

extensions/mongoProcessor/MongoQueueProcessor.js

+            groupId: `${this.mongoProcessorConfig.groupId}`,
+            // Must always have concurrency of 1 so writes are in order
+            kafka: { hosts: '127.0.0.1:9092' },
+            concurrency: 1,


I'm rather confident we can get rid of this limit for ingestion (likely cut&paste left-over, will remove w/ @rahulreddy 's benediction)

jonathan-gramain · 2018-06-30T22:13:37Z

extensions/mongoProcessor/MongoQueueProcessor.js

+            topic: this.mongoProcessorConfig.topic,
+            groupId: `${this.mongoProcessorConfig.groupId}`,
+            // Must always have concurrency of 1 so writes are in order
+            kafka: { hosts: '127.0.0.1:9092' },


This has to be configured via the global config object. Will fix

bert-e · 2018-07-01T00:05:10Z

Build failed

The build for commit did not succeed in branch feature/ZENKO-266-Ingestion-Consumer.

Co-authored-by: Lauren Spiegel <lhspiegel@gmail.com>

bert-e · 2018-07-01T00:53:17Z

Queue out of order

The changeset has received all authorizations to enter the merge queue,
however the merge queue is currently in an incoherent state. In order
to protect this pull-request, I have have not added the changeset to the
queue.

Please contact a member of release engineering.

JianqinWang mentioned this pull request Jun 25, 2018

ft: Zenko-266 mongo processor #261

Closed

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 3b6789c to be1c564 Compare June 27, 2018 00:29

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from be1c564 to 21fb915 Compare June 27, 2018 00:35

jonathan-gramain requested changes Jun 27, 2018

View reviewed changes

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 0f9b283 to ebf4831 Compare June 28, 2018 17:53

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from db9f42b to cf775c3 Compare June 29, 2018 18:07

rahulreddy added the high priority label Jun 29, 2018

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 3767165 to 1b29fd8 Compare June 30, 2018 00:36

JianqinWang force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 1b29fd8 to 5c3dae7 Compare June 30, 2018 00:45

rahulreddy force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 4f104e6 to 2676ffd Compare June 30, 2018 08:47

rahulreddy force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 2676ffd to da9f219 Compare June 30, 2018 09:06

jonathan-gramain force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from da9f219 to 8cdb0f7 Compare June 30, 2018 21:59

philipyoo reviewed Jun 30, 2018

View reviewed changes

jonathan-gramain reviewed Jun 30, 2018

View reviewed changes

jonathan-gramain force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 0952327 to 69343e4 Compare June 30, 2018 23:38

jonathan-gramain approved these changes Jun 30, 2018

View reviewed changes

philipyoo approved these changes Jun 30, 2018

View reviewed changes

ft: Zenko-266 mongo processor

2c5b0a6

Co-authored-by: Lauren Spiegel <lhspiegel@gmail.com>

jonathan-gramain force-pushed the feature/ZENKO-266-Ingestion-Consumer branch from 69343e4 to 2c5b0a6 Compare July 1, 2018 00:46

jonathan-gramain merged commit f579340 into development/8.0 Jul 1, 2018

jonathan-gramain deleted the feature/ZENKO-266-Ingestion-Consumer branch July 1, 2018 00:54

Feature/zenko 266 ingestion consumer #332

Feature/zenko 266 ingestion consumer #332

Conversation

JianqinWang commented Jun 25, 2018

bert-e commented Jun 25, 2018

Hello JianqinWang,

bert-e commented Jun 25, 2018

Waiting for approval

ironman-machine commented Jun 25, 2018

ironman-machine commented Jun 26, 2018

ironman-machine commented Jun 27, 2018

ironman-machine commented Jun 27, 2018

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

ironman-machine commented Jun 28, 2018

ironman-machine commented Jun 28, 2018

bert-e commented Jun 28, 2018

Conflict

ironman-machine commented Jun 28, 2018

bert-e commented Jun 28, 2018

Waiting for approval

ironman-machine commented Jun 28, 2018

ironman-machine commented Jun 28, 2018

ironman-machine commented Jun 29, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

ironman-machine commented Jun 30, 2018

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

bert-e commented Jul 1, 2018

Build failed

bert-e commented Jul 1, 2018

Queue out of order