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

Add configurable burst filter time #17

Merged

Conversation

henrique-silva
Copy link
Contributor

This PR introduces a way to configure the burst filter timeout in runtime, thus allowing users to match the burst filter to their application needs.

Introduces one new function for each logger implementation:

  • caPutLogSetBurstTimeout(<timeout>)
  • caPutJsonLogSetBurstTimeout(<timeout>)

This parameter can also be passed in the initialization or reconfiguration functions:

  • caPutLogInit(<address>[:port], <interest level>, <timeout>)
  • caPutLogReconf(<interest level>, <timeout>)
  • caPutJsonLogInit(<address>[:port], <interest level>, <timeout>)
  • caPutJsonLogReconf(<interest level>, <timeout>)

Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

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

I agree with the intention of this PR, but the implementation has some minor things I'd like to see changed including having a single definition of the default burst timeout. Inline comments below.

caPutLogApp/caPutJsonLogTask.h Show resolved Hide resolved
caPutLogApp/caPutLogTask.c Outdated Show resolved Hide resolved
test/caPutJsonLogTest.cpp Show resolved Hide resolved
Parameter can be specified on caPutLogInit, caPutLogReconf or
independently during runtime by caPutLogBurstTimeout()
@simon-ess simon-ess force-pushed the configurable-burst-time branch 3 times, most recently from 1e0d14d to 98b12b5 Compare July 22, 2024 09:27
henrique-silva and others added 3 commits July 22, 2024 12:57
This seems to be a bit awkward to get to compile for older c++ versions;
for c++11, you can use
```c
static constexpr double default_burst_timeout = 5.0;
```
but for older c++ (with no `constexpr`) this is not valid; moreover, both of
```c
const double default_burst_timeout = 5.0;
static const double default_burst_timeout = 5.0;
```
fail for different reasons. Given that this timeout is only used in one place
this seems like the simplest fix.
@simon-ess
Copy link
Contributor

@anjohnson All of the issues should be addressed. I ended up directly assigning the #defined value instead of the class having a const variable; I couldn't obviously get all of the different compilers in use to agree on how best to do that:

const double default_burst_timeout = 5.0;
static const double default_burst_timeout = 5.0;
static constexpr double default_burst_timeout = 5.0;

all fail in different cases. Who knew floats were such an issue? 🙃

@anjohnson
Copy link
Member

One more request (sorry!); please document the new iocsh commands & arguments here and add a release note entry mentioning the changes.

If this PR hasn't been rebased recently you'll want to do that first, I added release notes for the other recent changes last week.

@simon-ess
Copy link
Contributor

Documentation and release notes updated.

Copy link
Member

@anjohnson anjohnson left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to go now.

@anjohnson anjohnson merged commit a0d22e2 into epics-modules:master Jul 23, 2024
5 checks passed
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.

3 participants