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

max_transactions (i.e. mempool) should be a config option rather than hardcoded #3556

Closed
RichardAH opened this issue Jul 21, 2020 · 5 comments
Assignees
Labels
Good First Issue Great issue for a new contributor Tech Debt Non-urgent improvements

Comments

@RichardAH
Copy link
Collaborator

https://github.com/ripple/rippled/blob/7b048b423e8ae08a54018a89231d050b9f562855/src/ripple/overlay/impl/PeerImp.cpp#L1510-L1514

Not sure why this isn't a config option already?

@nbougalis
Copy link
Contributor

It probably should be.

I think it'd be fine to have "hard" limits in the code, and allow server operators to adjust "soft" limits within hard limits. For example:

/* These may not belong here - having this logic in the config may make more sense */
constexpr int minJQTxCount = 100;
constexpr int maxJQTxCount = 1000;

auto int limit = std::clamp(/* value from config */, minJQTxCount, maxJQTxCount);

if (app_.getJobQueue().getJobCount(jtTRANSACTION) > limit)

@nbougalis nbougalis added Good First Issue Great issue for a new contributor Tech Debt Non-urgent improvements labels Jul 22, 2020
@scorbajio
Copy link

I would like to work on this issue if no one else is actively working on it.

@carlhua
Copy link
Contributor

carlhua commented Aug 2, 2020

@ghorbanian go ahead! Thanks for volunteering!

@RichardAH
Copy link
Collaborator Author

I would like to work on this issue if no one else is actively working on it.

Cool! Easy fix ... should be easy to get a PR accepted. Clamp on new config value as nik suggested.

@nbougalis
Copy link
Contributor

I believe this has already been handled by @natenichols in #3562.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Great issue for a new contributor Tech Debt Non-urgent improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants