Skip to content

Introduce a potpourri of smaller improvements #2832

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

Merged
merged 19 commits into from
Jan 6, 2023

Conversation

dominiklohmann
Copy link
Member

This is a set of improvements seemingly unrelated to one another that amassed in one branch locally whenever I was traveling this vacation season.

These are the notable changes:

  1. Plugin names are now case-insensitive
  2. VASTQL is now a query-language plugin (builtin) so it no longer requires special-casing; this will allow for making the query language choice explicit in the future
  3. Removed PCAP-specific logic from the export command
  4. Removed handling of old deprecated options since the next release is going to be a major release
  5. VAST now shuts down instantly when metrics are enabled instead of being held alive for up to the duration of the telemetry interval (10s)

(1), (4), and (5) probably need changelog entries.

Not urgent to review, I won't be back in action before next week anyways, and it might very well be the case that I push another few things onto this. :)

This makes it easier to add additional logic to query parsing in the
future, as now query parsing is centralized in the plugin API.
This whole early expression-modifying approach for queries with the PCAP
writer attached didn't work properly as soon as a pipeline was attached
anyways, so we might as well remove it. We already had the correct check
in place within the writer, and this essentially just improves its error
message.
Now that we have broken anyways the next release will have to be a major
release—so let's also clean up older deprecation warnings while we're at
it.
To review this, run:

```bash
vast --bare-mode --enable-metrics --plugins= start --commands=stop
```

Before this change, this took exactly 10s to shut down as the telemetry loops
held some actors alive with strong references on the actor clock's scheduler.
@dominiklohmann dominiklohmann force-pushed the topic/random-vacation-improvements branch from 95fcc1f to 6953dd0 Compare January 4, 2023 01:26
@dominiklohmann dominiklohmann force-pushed the topic/random-vacation-improvements branch from 6953dd0 to 6958bb2 Compare January 4, 2023 01:45
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Neat stuff.

I know the PR is still an draft mode, but it was very reviewable already. 🙂

@dominiklohmann dominiklohmann marked this pull request as ready for review January 5, 2023 19:51
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Nice touch ups. 🌴

@dominiklohmann dominiklohmann merged commit 8bb723c into master Jan 6, 2023
@dominiklohmann dominiklohmann deleted the topic/random-vacation-improvements branch January 6, 2023 02:16
mavam added a commit that referenced this pull request Jan 7, 2023
This PR updates the documentation after VASTQL became its own language
frontend in #2832.
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