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

Maximum Transactions in JobQueue read from Config #3562

Closed

Conversation

natenichols
Copy link
Contributor

@natenichols natenichols commented Jul 27, 2020

High Level Overview of Change

Fixes #3556. max_transactions is now read from the rippled.cfg instead of hardcoded.

Context of Change

The maximum capacity of JobQueue was hardcoded in PeerImp.cpp. Now the maximum capacity is read from the configuration file. max_transactions defaults to 250, and has a hardcoded minimum of 100 and a hardcoded maximum of 1000, as suggested in the issue description.

This option is added to the configuration with:

[max_transactions]
    500

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Before/After

Before:
max_transactions was hardcoded.

After:
MAX_TRANSACTIONS is read from rippled.cfg in Config.cpp. std::clamp is used to ensure the configuration value is between 100 and 1000. Member variable max_transactions_ was created in PeerImp, and initialized to the value read by the configuration.

Test Plan

All unit tests pass. No tests added.

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for your first contribution and also for being mindful enough to edit the example configuration file to reflect the new tunable.

I left a small comment that should be trivial to address.

You should also consider renaming your commit message to adhere to best practices. I recommend:

Make the transaction job queue limit adjustable

// The maximum number of transactions to have in the job queue.
constexpr int max_transactions = 250;
if (app_.getJobQueue().getJobCount(jtTRANSACTION) > max_transactions)
if (app_.getJobQueue().getJobCount(jtTRANSACTION) > max_transactions_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider instead:

if (app_.getJobQueueCount().getJobCount(jtTRANSACTION) > app_.config().MAX_TRANSACTIONS)

Rationale: the variable is only set in one place and isn't "per-peer" so this lets you eliminate the unnecessary member variable, and highlights the intent of the code.

@natenichols natenichols requested a review from nbougalis July 30, 2020 03:28
@natenichols natenichols force-pushed the max-transaction-config branch from c2b8857 to 009c527 Compare July 30, 2020 04:15
Copy link
Contributor

@pwang200 pwang200 left a comment

Choose a reason for hiding this comment

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

Looks good and works fine!

@carlhua carlhua added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max_transactions (i.e. mempool) should be a config option rather than hardcoded
4 participants