-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ft: ZENKO-583 get CRR status #331
ft: ZENKO-583 get CRR status #331
Conversation
0da2585
to
61c09d3
Compare
PR has been updated. Reviewers, please be cautious. |
61c09d3
to
8d14038
Compare
PR has been updated. Reviewers, please be cautious. |
9dc16cf
to
cd8ca97
Compare
8d14038
to
ccb0c63
Compare
PR has been updated. Reviewers, please be cautious. |
5a75051
to
b8502e2
Compare
ccb0c63
to
c817a92
Compare
PR has been updated. Reviewers, please be cautious. |
58fbbe4
to
f288387
Compare
docs/crr-pause-resume.md
Outdated
When the paused Consumer is resumed, it will again resume consuming entries | ||
from its last offset. | ||
QueueProcessors will subscribe to this channel, and on receiving a request | ||
to pause or resume CRR, will notify all of its BackbeatConsumers to perform the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of their BackbeatConsumers
docs/crr-pause-resume.md
Outdated
all sites configured as destination replication endpoints. | ||
|
||
Response: | ||
```sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use json
not sh
.
docs/crr-pause-resume.md
Outdated
all locations configured as destination replication endpoints. | ||
|
||
Response: | ||
```sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use json
not sh
.
docs/crr-pause-resume.md
Outdated
Response: | ||
```sh | ||
{ | ||
location1: "disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the output shall be JSON, let's add double-quotes to the keys like "location1"
docs/crr-pause-resume.md
Outdated
Response: | ||
```sh | ||
{ | ||
location: "enabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be better to stick with the name that appears in the syntax line: "<location-name>": "enabled"
if (err && err.name === 'NODE_EXISTS') { | ||
zkClient.getData(path, (err, data) => { | ||
if (err) { | ||
log.fatal('could not check check site status in zookeeper', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check check
zkClient.connect(); | ||
zkClient.once('error', err => { | ||
log.fatal('error connecting to zookeeper', { | ||
error: err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: err.message
log.fatal('could not create path in zookeeper', { | ||
method: 'QueueProcessor:task', | ||
zookeeperPath: path, | ||
error: err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
lib/BackbeatConsumer.js
Outdated
if (!disableConsumer) { | ||
this._consumer.subscribe([this._topic]); | ||
} else { | ||
this._log.info(`consumer not subcribed to topic ${this._topic}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this level I would rather log in debug mode, as the message is likely obscure for a normal user. Such message would better fit at the service level (stating that it's paused, for example).
lib/api/BackbeatAPI.js
Outdated
@@ -236,7 +239,9 @@ class BackbeatAPI { | |||
getHealthcheck(details, cb) { | |||
return this._healthcheck.getHealthcheck((err, data) => { | |||
if (err) { | |||
this._logger.error('error getting healthcheck', err); | |||
this._logger.error('error getting healthcheck', { | |||
error: err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error: err.message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, this is my mistake, I want to pass err
here as is because its a unique object returned from the Healthcheck class. I'll look into figuring out a better way of implementing this in a future refactor.
Change is reverted
c817a92
to
cdcc885
Compare
PR has been updated. Reviewers, please be cautious. |
1 similar comment
PR has been updated. Reviewers, please be cautious. |
|
PR has been updated. Reviewers, please be cautious. |
ee01f6d
to
0bb1482
Compare
PR has been updated. Reviewers, please be cautious. |
0bb1482
to
51155a7
Compare
PR has been updated. Reviewers, please be cautious. |
1 similar comment
PR has been updated. Reviewers, please be cautious. |
|
c3f8525
to
6896d6b
Compare
PR has been updated. Reviewers, please be cautious. |
Hello philipyoo,My role is to assist you with the merge of this Status report is not available. |
ConflictThere is a conflict between your branch Please resolve the conflict on the feature branch (
|
d381aa7
to
5a3349e
Compare
PR has been updated. Reviewers, please be cautious. |
5a3349e
to
2155857
Compare
PR has been updated. Reviewers, please be cautious. |
2155857
to
2d3a30f
Compare
PR has been updated. Reviewers, please be cautious. |
2d3a30f
to
74d91ae
Compare
PR has been updated. Reviewers, please be cautious. |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a |
bert-e help |
@bert-e help |
Help pageThe following options and commands are available at this time. Options
Commands
|
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
PR has been updated. Reviewers, please be cautious. |
|
d007e52
to
dcd1b14
Compare
PR has been updated. Reviewers, please be cautious. |
@bert-e reset |
Check if CRR is enabled for a given destination replication location via the API.
Save CRR status state within zookeeper. If pod goes down, read and resume from previous state