Opened 3 years ago
Closed 22 months ago
#20549 closed enhancement (implemented)
Make bridge descriptor sanitization more maintainable and more modular
Reported by: | iwakeh | Owned by: | metrics-team |
---|---|---|---|
Priority: | High | Milestone: | |
Component: | Metrics/CollecTor | Version: | |
Severity: | Normal | Keywords: | metrics-2018 |
Cc: | Actual Points: | ||
Parent ID: | #20542 | Points: | |
Reviewer: | iwakeh | Sponsor: |
Description
SanitizedBridgeServerDescriptor
should extend BridgeServerDescriptor
.
It receives a BridgeServerDescriptor
at construction time and sanitizes the descriptor.
All getters should only supply sanitized data; this explicitely includes method from the Descriptor interface like getRawDescriptorBytes.
In addition, a SanitizedBridgeServerDescriptorTest
should be created that verifies the sanitation and data integrity.
Child Tickets
Change History (13)
comment:1 Changed 2 years ago by
Summary: | implement SanitizedBridgeServerDescriptor → Implement SanitizedBridgeServerDescriptor class that encapsulates the sanitizing logic for bridge server descriptors |
---|
comment:2 Changed 2 years ago by
Keywords: | metrics-2018 added |
---|
comment:3 Changed 2 years ago by
Owner: | set to metrics-team |
---|---|
Status: | new → assigned |
comment:4 follow-up: 5 Changed 2 years ago by
Also pay attention to the issues mentioned in #23981's comments.
Shouldn't this be moved to metrics-2017?
comment:5 follow-up: 6 Changed 2 years ago by
Keywords: | metrics-2017 added; metrics-2018 removed |
---|
Replying to iwakeh:
Also pay attention to the issues mentioned in #23981's comments.
Right. In particular, I'd like to try out the following change that I suggested on that ticket:
"While working on this patch I considered taking a different approach where we'd include placeholders for parts that we cannot sanitize yet and fill them in after finishing to read the original descriptor. This could be a List<StringBuilder>
to retain order of original lines and where we'd keep references to some StringBuilder
s that we need to sanitize after reading. Still something to think about. Let's do that after fixing this ugly bug."
Shouldn't this be moved to metrics-2017?
Sure. Worth trying.
comment:6 Changed 2 years ago by
Status: | assigned → needs_review |
---|
Replying to karsten:
"While working on this patch I considered taking a different approach where we'd include placeholders for parts that we cannot sanitize yet and fill them in after finishing to read the original descriptor. This could be a
List<StringBuilder>
to retain order of original lines and where we'd keep references to someStringBuilder
s that we need to sanitize after reading. Still something to think about. Let's do that after fixing this ugly bug."
I started an implementation in my task-20549 branch that doesn't get us all the way to implementing this ticket yet, but which may be a start. Feedback welcome!
comment:8 Changed 2 years ago by
Keywords: | metrics-2018 added; metrics-2017 removed |
---|
Will be completed in 2018.
comment:9 Changed 22 months ago by
Priority: | Medium → High |
---|
Raising priority, as this may soon become more relevant when reprocessing bridge descriptors.
comment:10 Changed 22 months ago by
This s a different approach from the tickets description. But, I really like this idea and added seven more commits. The commit comments should describe the details, if necessary. The general changes are some name changes, like introducing DescriptorBuilder
as builder, which we might reuse elsewhere.
The new DescriptorBuilder
also accepts appending DescriptorBuilder
now.
Please review seven commits on the task-20549 branch.
Maybe, we should change the title and summary to a more general "Make bridge descriptor sanitization more maintainable and more modular"?
comment:11 Changed 22 months ago by
Status: | needs_review → needs_information |
---|
Changing to needs_information also meaning needs_review.
comment:12 Changed 22 months ago by
Status: | needs_information → merge_ready |
---|---|
Summary: | Implement SanitizedBridgeServerDescriptor class that encapsulates the sanitizing logic for bridge server descriptors → Make bridge descriptor sanitization more maintainable and more modular |
Great! Your additional changes look good to me. I made two tiny changes in my task-20549 branch (this.
and a whitespace change), squashed, rebased, and pushed to master. Setting status to merge_ready.
Do we want to make further changes as part of this ticket? If so, can you start a list of next steps here? If not, can you close this ticket? Thanks!
comment:13 Changed 22 months ago by
Resolution: | → implemented |
---|---|
Status: | merge_ready → closed |
Changes look ok, thanks!
Well, there'll always be more steps, but let's call this ticket finished.
I created #25307 as I'm now aware that this should be quite easy (just a question to clarify on that ticket).
Closing.
Thanks!
Try to make the summary slightly more useful.