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

Revert "Annotate schemas with #index=hash" #765

Merged
merged 3 commits into from Feb 25, 2020
Merged

Conversation

lava
Copy link
Member

@lava lava commented Feb 21, 2020

This partially reverts commit 9f656e8.

When benchmarking, we discovered that the hash index introduces
a huge performance regression for the vast import suricata
command, due to a variant type being used as a key in a flat_map.

Therefore, we're removing the hash indices until the underlying
problem is fixed.

@lava lava changed the title Partially revert "Annotate schemas with #inde..." Revert "Annotate schemas with #index=hash" Feb 21, 2020
@tobim
Copy link
Member

tobim commented Feb 21, 2020

If we want to disable the hash index completely, we also need to disable the attribute inject heuristic in the Zeek parser.

We should also write a changelog entry so users know why this index is disabled.

Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This looks fine, but please address the changes requested by @tobim.

lava and others added 2 commits February 24, 2020 09:43
This reverts commit 9f656e8.

When benchmarking, we discovered that the hash index introduces
a huge performance regression for the `vast import suricata`
command, due to a variant type being used as a key in a `flat_map`.

Therefore, we're removing the hash indices until the underlying
problem is fixed.
This reverts commit cc1b8e6.

Remove hash indices due to performance regressions.
@lava lava added performance Improvements or regressions of performance bug Incorrect behavior labels Feb 24, 2020
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This is fine as is. Please apply the suggested cosmetic fixes.

CHANGELOG.md Outdated Show resolved Hide resolved
schema/argus.schema Show resolved Hide resolved
@lava lava merged commit a60fc23 into master Feb 25, 2020
@lava lava deleted the topic/hash-index-revert branch February 25, 2020 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior performance Improvements or regressions of performance
Projects
None yet
3 participants