-
Notifications
You must be signed in to change notification settings - Fork 163
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
Keep segments containing ISD-level loops #2046
Conversation
9d72ea8
to
36eda1b
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @worxli)
python/beacon_server/core.py, line 44 at r1 (raw file):
filter_ISD
I would name it filter_isd_loops
then it is immediately clear what it filters.
Note that I think ISD should be lower case in the variable name, as in the rest of the code we also have stuff in lower case (isd_as
, if_id
)
python/beacon_server/core.py, line 160 at r1 (raw file):
# Switched to a new ISD last_isd = curr_isd if curr_isd in isds and self.filter_ISD:
I would put the bool check in front of the list check since it is cheaper.
9972a6a
to
90f20d7
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)
python/beacon_server/core.py, line 44 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
filter_ISD
I would name it
filter_isd_loops
then it is immediately clear what it filters.
Note that I think ISD should be lower case in the variable name, as in the rest of the code we also have stuff in lower case (isd_as
,if_id
)
Done.
python/beacon_server/core.py, line 160 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
I would put the bool check in front of the list check since it is cheaper.
Done.
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.
Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @worxli)
python/bin/beacon_server, line 41 at r2 (raw file):
'(Default: %s)' % get_default_sciond_path()) parser.add_argument('--filter_isd_loops', default=False, help='Filter ISD loops in Core Beacon Server (Default: False)')
As discussed offline please use action='store_true'
so that we can use it without value (only flag --filter_isd_loops
)
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.
Reviewed 1 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli)
b7795b0
to
15d5085
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.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @worxli)
python/bin/beacon_server, line 40 at r3 (raw file):
default=False,
This is no longer 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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
python/bin/beacon_server, line 40 at r3 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
default=False,
This is no longer needed.
Done.
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.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 3 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
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.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all files reviewed, all discussions resolved
Fixes #2037
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)