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

user configurable event buffer size #4411

Merged
merged 1 commit into from
Dec 2, 2021
Merged

user configurable event buffer size #4411

merged 1 commit into from
Dec 2, 2021

Conversation

iknox-fa
Copy link
Contributor

@iknox-fa iknox-fa commented Dec 2, 2021

resolves # No ticket-- follow on to #4358

Description

Add new global cli flag EVENT_BUFFER_SIZE to control the number of events buffered in memory during operation.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Dec 2, 2021
# if and only if the event history deque will be completely filled by this event
# fire warning that old events are now being dropped
global EVENT_HISTORY
if len(EVENT_HISTORY) == ((EVENT_HISTORY.maxlen or 100000) - 1):
if len(EVENT_HISTORY) == (flags.EVENT_BUFFER_SIZE - 1):
Copy link
Contributor Author

@iknox-fa iknox-fa Dec 2, 2021

Choose a reason for hiding this comment

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

@nathaniel-may Sorry I missed this in the first PR feedback, but this pattern is required or fire_event(EventBufferFull()) will create a recursion loop. :|

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh right.

@@ -88,25 +89,29 @@ def test_event_codes(self):

class TestEventBuffer(TestCase):

def setUp(self) -> None:
flags.EVENT_BUFFER_SIZE = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

much better

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

LGTM

@iknox-fa iknox-fa marked this pull request as ready for review December 2, 2021 22:37
@iknox-fa iknox-fa requested a review from emmyoop December 2, 2021 22:37
@iknox-fa
Copy link
Contributor Author

iknox-fa commented Dec 2, 2021

@nathaniel-may @jtcohen6 -- Is there anything else additional I need to do to document the new flag? I'm not sure of the SOP for new ones.

@nathaniel-may
Copy link
Contributor

good question. I'm not sure. Maybe @leahwicz knows?

@leahwicz
Copy link
Contributor

leahwicz commented Dec 2, 2021

I'm not sure but let's merge this PR and we can always open another PR for docs

@emmyoop
Copy link
Member

emmyoop commented Dec 2, 2021

main.py has the help strings for the CLI flags.

@iknox-fa iknox-fa merged commit 2cd1f7d into main Dec 2, 2021
@iknox-fa iknox-fa deleted the event_buffer_config branch December 2, 2021 22:47
leahwicz pushed a commit that referenced this pull request Dec 2, 2021
leahwicz added a commit that referenced this pull request Dec 2, 2021
Co-authored-by: Ian Knox <81931810+iknox-fa@users.noreply.github.com>
iknox-fa added a commit that referenced this pull request Feb 8, 2022
automatic commit by git-black, original commits:
  2cd1f7d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants