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

enhancement(enriching): enable enrichment file reload on SIGHUP #9371

Merged
merged 5 commits into from
Oct 8, 2021

Conversation

StephenWakely
Copy link
Contributor

@StephenWakely StephenWakely commented Sep 29, 2021

This allows the enrichment files to reload the data when Vector receives a SIGHUP.

The implementation may raise some eyebrows as I have put the main functionality into the clone method of the File struct.

The issue is that on reload I want to either clone the object or create a new object with the reloaded data. Since enrichment tables are represented in Vector as Box<dyn Table>, the trait can't return Self. Clone currently works because of dyn_clone which pulls a number of unsafe tricks in order to work.

In order to perform the reload Table also needs to return Self in order to return the object representing the reloaded data. Since Clone is the only way to do that, I have had to put the reload functionality there.

It doesn't sit 100% well with me, so if there are any other suggestions I would appreciate it.

The reloading now occurs when the config is reloaded, we can check if the underlying data has changed and then load a new enrichment table. When adding this to the registry, it will overwrite any that have been reloaded.

Signed-off-by: Stephen Wakely fungus.humungus@gmail.com

Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
@netlify
Copy link

netlify bot commented Sep 29, 2021

✔️ Deploy Preview for vector-project ready!

🔨 Explore the source changes: 639a7ce

🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/615edb593f0fea0008b75b0e

😎 Browse the preview: https://deploy-preview-9371--vector-project.netlify.app

Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
@StephenWakely StephenWakely mentioned this pull request Oct 1, 2021
17 tasks
Copy link
Contributor

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

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

The Clone solution does seem somewhat unorthodox and likely to confuse people.

What if we instead moved "reloading" up a level, and instead reload the entire VRL stack (read: re-compile the program and re-initialize the runtime) when Vector reloads? That way, a new table could be initialized on reload, instead of having to do the clone-trick being done right now?

It would also have the benefit of gaining another reload-feature: changing a VRL program and having it work after a SIGHUP.

@StephenWakely
Copy link
Contributor Author

What if we instead moved "reloading" up a level, and instead reload the entire VRL stack

The reload here isn't occurring withing VRL, it is the enrichment tables. Reloading VRL wouldn't have any impact. I imagine VRL is already reloaded if the transform is modified?

@JeanMertz
Copy link
Contributor

The reload here isn't occurring withing VRL, it is the enrichment tables. Reloading VRL wouldn't have any impact. I imagine VRL is already reloaded if the transform is modified?

Aha, gotcha. I'm actually not that familiar with the reloading logic, so I guess if it already "restarts" any changed component, then yes, it would work out-of-the-box for any VRL-powered component.

I don't have any alternatives for you unfortunately, but maybe someone else does, it definitely feels like we're solving this in the wrong layer by having to do this within a Clone impl, but I haven't done a deep-dive, so it might just be we have to accept this trade-off.

@StephenWakely
Copy link
Contributor Author

Thinking about it I think I can do it in multiple stages:

  1. Ask the table to check if it needs reloading.
  2. If it does need reloading ask the table to clear out it's data if it is going to need reloading.
  3. Clone the table (this will be cheap now the data has been cleared out).
  4. Get the table to reload the data if it needed reloading.

@StephenWakely
Copy link
Contributor Author

StephenWakely commented Oct 4, 2021

  1. If it does need reloading ask the table to clear out it's data if it is going to need reloading.

Actually, no this doesn't work. We can't clear out the data before cloning because that would be clearing out the data that is currently in use and is readonly. We would have to clone all the data to a new record before clearing out, which would be a completely unneccesary copying of data..

Thus intercepting this at the point of clone is still the only way I can see around this..

Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
Copy link
Contributor

@JeanMertz JeanMertz left a comment

Choose a reason for hiding this comment

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

sweet!

@StephenWakely StephenWakely merged commit d0a8cba into master Oct 8, 2021
@StephenWakely StephenWakely deleted the stephen/enrichment-reload branch October 8, 2021 10:21
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