Skip to content
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

bf: BB-419 increase notification default concurrency #2422

Merged

Conversation

jonathan-gramain
Copy link
Contributor

@jonathan-gramain jonathan-gramain commented Jun 14, 2023

Replaces #2421

  • increase default notification BackbeatConsumer concurrency

    Notification QueueProcessor was limited by a concurrency of 10, which meant it could only wait for 10 delivery reports from the producer before consuming more entries.

    Increased the default concurrency to 1000 to improve throughput and add a test to show that it behaves well with this number of pending reports (the processing does not involve anything else asynchronous than waiting for delivery reports), as well as a few other extra tests.

    Also fix and improve logging related to processing errors and error reporting from delivery reports.

  • [cleanup] BackbeatProducer.pollIntervalMs default as constant

    Use a code constant for pollIntervalMs, and add a JSDoc about it (and messageMaxBytes)

  • [impr] configurable notif Kafka poll interval

    Make the external Kafka producer poll interval configurable: while the default is 2000ms, it could be helpful to reduce its value, e.g. to reduce latency of delivery in case the external Kafka cluster cannot receive all notifications in time.

    It's not clear at this point what would be a better value due to lack of testing to the limits, and 2000ms doesn't look unreasonable as a trade-off between latency and polling overhead, so leaving the default value as it is. It is however important to have a concurrency value high enough to not limit the possible throughput per notification processor, which is limited by "concurrency / poll_interval_seconds" messages per second. The concurrency has been raised to 1000 by default which therefore allows 500 messages per second to go through per processor with a 2000ms poll interval.

@bert-e
Copy link
Contributor

bert-e commented Jun 14, 2023

Hello jonathan-gramain,

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

Status report is not available.

@bert-e
Copy link
Contributor

bert-e commented Jun 14, 2023

Conflict

A conflict has been raised during the creation of
integration branch w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency with contents from bugfix/BB-419-increaseNotificationDefaultConcurrency
and development/7.70.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency origin/development/7.70
 $ git merge origin/bugfix/BB-419-increaseNotificationDefaultConcurrency
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency

@jonathan-gramain jonathan-gramain force-pushed the bugfix/BB-419-increaseNotificationDefaultConcurrency branch from f801b51 to 27b4046 Compare June 14, 2023 22:42
}, err => {
assert.ifError(err);
// should not have been sent
assert(!send.calledOnce);
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be true if send.calledTwice == true? Is .notCalled what is wanted instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, using send.notCalled

done();
});
});
});
Copy link
Contributor

@watsaqat watsaqat Jun 16, 2023

Choose a reason for hiding this comment

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

is it possible that JSON parsing might fail in qp.processKafkaEntry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually throws an exception if the input is not valid JSON.

It's not directly related to the scope of this PR, but it can be a good idea to return a proper error instead and add a test case for it indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed JSON parsing exception and added a test

// no "eventType"
value: '{}',
}),
}, err => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's no eventType, is there an error/log message or it just silently doesn't send?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is where the check happens: https://github.com/scality/backbeat/blob/development/8.6/extensions/notification/utils/config.js#L85

I believe there is no particular log.

For the context, I closed a previous iteration of this PR (#2421) that was not waiting for the callback of send but we decided to rework it differently, the point of this test was then mostly to trigger a case where a message is not sent for regression testing, not particularly to check all details of the particular case where eventType is not passed. I kept it because I think it has some value to test this case but it's not anymore directly related to the change.

Notification QueueProcessor was limited by a concurrency of 10, which
meant it could only wait for 10 delivery reports from the producer
before consuming more entries.

Increased the default concurrency to 1000 to improve throughput and
add a test to show that it behaves well with this number of pending
reports (the processing does not involve anything else asynchronous
than waiting for delivery reports), as well as a few other extra
tests.

Also fix and improve logging related to processing errors and error
reporting from delivery reports.
Use a code constant for `pollIntervalMs`, and add a JSDoc about it
(and `messageMaxBytes`)
Make the external Kafka producer poll interval configurable: while the
default is 2000ms, it could be helpful to reduce its value, e.g. to
reduce latency of delivery in case the external Kafka cluster cannot
receive all notifications in time.

It's not clear at this point what would be a better value due to lack
of testing to the limits, and 2000ms doesn't look unreasonable as a
trade-off between latency and polling overhead, so leaving the default
value as it is. It is however important to have a concurrency value
high enough to not limit the possible throughput per notification
processor, which is limited by "concurrency / poll_interval_seconds"
messages per second. The concurrency has been raised to 1000 by
default which therefore allows 500 messages per second to go through
per processor with a 2000ms poll interval.
@jonathan-gramain jonathan-gramain force-pushed the bugfix/BB-419-increaseNotificationDefaultConcurrency branch from 27b4046 to 847f219 Compare July 10, 2023 23:29
@bert-e
Copy link
Contributor

bert-e commented Jul 10, 2023

Incorrect fix version

The Fix Version/s in issue BB-419 contains:

  • 7.10.12

  • 7.70.3

  • 8.4.26

  • 8.5.5

  • 8.6.23

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 7.10.12

  • 7.70.3

  • 8.5.5

  • 8.6.26

  • 8.7.0

Please check the Fix Version/s of BB-419, or the target
branch of this pull request.

@bert-e
Copy link
Contributor

bert-e commented Jul 10, 2023

Conflict

A conflict has been raised during the creation of
integration branch w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency with contents from bugfix/BB-419-increaseNotificationDefaultConcurrency
and development/7.70.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency origin/development/7.70
 $ git merge origin/bugfix/BB-419-increaseNotificationDefaultConcurrency
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency

@bert-e
Copy link
Contributor

bert-e commented Jul 11, 2023

Conflict

A conflict has been raised during the creation of
integration branch w/8.5/bugfix/BB-419-increaseNotificationDefaultConcurrency with contents from w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency
and development/8.5.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/8.5/bugfix/BB-419-increaseNotificationDefaultConcurrency origin/development/8.5
 $ git merge origin/w/7.70/bugfix/BB-419-increaseNotificationDefaultConcurrency
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/8.5/bugfix/BB-419-increaseNotificationDefaultConcurrency

@bert-e
Copy link
Contributor

bert-e commented Jul 11, 2023

Integration data created

I have created the integration data for the additional destination branches.

The following branches will NOT be impacted:

  • development/7.4

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

@bert-e
Copy link
Contributor

bert-e commented Jul 11, 2023

Waiting for approval

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

  • the author

  • 2 peers

@bert-e
Copy link
Contributor

bert-e commented Jul 11, 2023

History mismatch

Merge commit #1e5dae878ab6d5cf220e8460791d515480d132c1 on the integration branch
w/8.6/bugfix/BB-419-increaseNotificationDefaultConcurrency is merging a branch which is neither the current
branch bugfix/BB-419-increaseNotificationDefaultConcurrency nor the development branch
development/8.6.

It is likely due to a rebase of the branch bugfix/BB-419-increaseNotificationDefaultConcurrency and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

@bert-e
Copy link
Contributor

bert-e commented Jul 12, 2023

Waiting for approval

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

  • the author

  • 2 peers

@jonathan-gramain
Copy link
Contributor Author

/approve

@bert-e
Copy link
Contributor

bert-e commented Jul 12, 2023

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

  • ✔️ development/7.10

  • ✔️ development/7.70

  • ✔️ development/8.5

  • ✔️ development/8.6

  • ✔️ development/8.7

The following branches will NOT be impacted:

  • development/7.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

  • Any commit you add on the source branch will trigger a new cycle after the
    current queue is merged.
  • Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

@bert-e
Copy link
Contributor

bert-e commented Jul 12, 2023

I have successfully merged the changeset of this pull request
into targetted development branches:

  • ✔️ development/7.10

  • ✔️ development/7.70

  • ✔️ development/8.5

  • ✔️ development/8.6

  • ✔️ development/8.7

The following branches have NOT changed:

  • development/7.4

Please check the status of the associated issue BB-419.

Goodbye jonathan-gramain.

@bert-e bert-e merged commit 847f219 into development/7.10 Jul 12, 2023
@bert-e bert-e deleted the bugfix/BB-419-increaseNotificationDefaultConcurrency branch July 12, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants