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

S3 refactor timer coros => change to store timer coros on flb_sched struct #28

Open
wants to merge 2 commits into
base: s3-refactor-timer-coros-awspr
Choose a base branch
from

Conversation

PettitWesley
Copy link
Owner


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

…t async timers on the sched

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
Signed-off-by: Wesley Pettit <wppttt@amazon.com>
void *sched;
struct flb_sched *sched;

Choose a reason for hiding this comment

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

Wonder why this is a void * in the first place. Seems like all pointers are void * in this file. I'd keep it void * unless we understand this convention and have a reason to break it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

void * is useful if at compile time it could store multiple different structs. Here its always one struct, so there's no good reason AFAIK for it to be void.

Comment on lines +94 to +95
/* Each event loop has a scheduler instance */
struct flb_sched *sched;

Choose a reason for hiding this comment

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

Calling out that this is the central change in this pr

Copy link

@matthewfala matthewfala left a comment

Choose a reason for hiding this comment

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

These changes look great! There may be a typo here: flb_async_timer_cleanup(config->sched->async_timer_list_destroy)

as it may need to be (config->sched)

@@ -929,7 +916,7 @@ int flb_engine_start(struct flb_config *config)
flb_net_dns_lookup_context_cleanup(&dns_ctx);
flb_sched_timer_cleanup(config->sched);
flb_upstream_conn_pending_destroy_list(&config->upstreams);
flb_output_async_timer_cleanup(config);
flb_async_timer_cleanup(config->sched->async_timer_list_destroy);

Choose a reason for hiding this comment

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

Typo? Should this be config->sched

mk_list_foreach_safe(head, tmp, destroy_list) {
async_timer = mk_list_entry(head, struct flb_out_async_timer, _head);
async_timer = mk_list_entry(head, struct flb_async_timer, _head);
Copy link
Owner Author

Choose a reason for hiding this comment

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

TODO: need to acquire mutex every time the engine reads or the worker thread writes/deletes.

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

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.

2 participants