-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Remove port type #1187
Remove port type #1187
Conversation
@mavam this needs the integration test baseline to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a first round. The code itself looks fine, but I'll do some git grep
locally to check whether you missed anything. Also still need to test locally.
2234463
to
a11f434
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran further tests locally and this is looking alright to me. Tested various queries and they all work as expected.
Please update the docs as noted in the checklist. This also must be rebased onto latest master before merging, and there's a missing newline in the changelog entry.
I'm yet unsure whether we want to bump the db version. Currently, we run into a global buffer overflow according to asan when loading an old db with this version.
Once approved, I'll pivot to updating docs.
✅ |
Now that the scope is wider, this reflects better the intention.
@tenzir/backend new DV version changes implemented as discussed. See last two commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed via Slack, there's a segfault when the db version mismatches. Just nits otherwise.
libvast/vast/layout_version.hpp
Outdated
/// This version number defines compatibility of persistent state with with | ||
/// prior directory layouts and contents. Breaking changes shall bump the | ||
/// version number. A breaking change includes the structure, sequence and | ||
/// organization of the database directory itself, as well irreconcilable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing some word before irreconcilable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, can you please use incompatible
instead? I had to look up what irreconcilable
even means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, works well now.
📔 Description
This PR removes the
port
type from the data model. Ports becomecount
types. To keep the domain-specific semantics when working with network data, a type alias with the nameport
will be the new way to express the same data.📝 Checklist
🎯 Review Instructions
File by file.