-
Notifications
You must be signed in to change notification settings - Fork 10
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
FOG-408: Make ingest peer uri filtering based on responder id #22
FOG-408: Make ingest peer uri filtering based on responder id #22
Conversation
Was there a successful deployment with this revision? |
not yet |
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.
LGTM once everything is green and happy. Let me know if you want to offload getting it there to me.
This fixes a bug that exists today in fog ingest deployments. I thought this was only a minor issue because, it would wrongly add the peer listen uri to the list of peer uris, but then filter it out and not connect to it. But because the active server replicates its peer list onto the backups continuously, they will all end up with this uri in their list, and there is no configuration that will fix this. Additionally, a server won't be able to filter out it's own (real) URI that the peers have in their lists, because it doesn't filter by responder id before this commit.
b1b680a
to
36dac5d
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.
LGTM once successful deploy
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.
It's fine as-is, but it really seems like the "is the responder ID valid" test should happen when creating an IngestPeerUri
, no? Is there ever a case where an IngestPeerUri
without a responder ID is valid?
no, i did not create the responder id API -- i made a ticket already to track making responder id infallible https://mobilecoin.atlassian.net/browse/MCC-2273 It sounds like we are broadly in agreement -- I think all that would have to happen to implement this ticket is,
|
@@ -38,7 +38,7 @@ pub struct IngestConfig { | |||
pub peer_listen_uri: IngestPeerUri, | |||
|
|||
/// List of all peers in this cluster | |||
#[structopt(long)] | |||
#[structopt(long, use_delimiter = true)] |
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.
Why was this needed?
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.
if you want to have it work like --peers igp://foo,igp://bar,igp://baz,igp://qaz
you need to put use_delimter = true
. Otherwise, when structopt sees a Vec, you must make it like --peers igp://foo --peers igp://baz --peers igp://qaz
and that just seems harder to read
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.
here's the issue in structopt repo: TeXitoi/structopt#94
This fixes a bug that exists today in fog ingest deployments.
I thought this was only a minor issue because, it would wrongly
add the peer listen uri to the list of peer uris, but then filter
it out and not connect to it. But because the active server replicates
its peer list onto the backups continuously, they will all end up
with this uri in their list, and there is no configuration that will
fix this. Additionally, a server won't be able to filter out it's
own (real) URI that the peers have in their lists, because it doesn't
filter by responder id before this commit.