Opened 3 years ago

Closed 3 years ago

#23215 closed defect (fixed)

set annotation from descriptor during sync-runs

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.3.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


As noticed in #21759:comment10

Currently, all synced descriptors receive their annotation from the Annotation enum (cf. package o.t.c.conf.Annotation). This happens, because only the raw bytes are taken from a given descriptor and written to the file system.
But, actually the annotation(s) should be taken from the 'getAnnotations' method and be prepended to the raw descriptor bytes, shouldn't they?

The fix for this would also simplify some sync-code.

Child Tickets

Change History (9)

comment:1 Changed 3 years ago by iwakeh

Status: newneeds_information

comment:2 Changed 3 years ago by iwakeh

Is the above approach ok?

comment:3 Changed 3 years ago by karsten

It does sound plausible, though I'd have to see the code to be sure.

comment:4 Changed 3 years ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: needs_informationaccepted

Fine, implementation patch follows.

comment:5 Changed 3 years ago by iwakeh

Please take a look at the two commits on top of this branch.

When the change and test data make sense I'll add more test data with currently valid annotations and the corresponding changelog entry.

comment:6 Changed 3 years ago by iwakeh

Status: acceptedneeds_review

comment:7 Changed 3 years ago by karsten

Looks good!

Nitpick: let's avoid abbreviations in variable names and write "annotations" instead of "annos" and "result" instead of "res". It'll cost 5 seconds more to write this code but save at least 5 seconds each time somebody reads this code.

comment:8 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

I added a little more test variety and the changelog entry as another commit to the above branch.

Regarding the nitpick: I find it easier to distinguish between "anno" and annos" than "annotation" and "annotations"; in general very long variable names feel/look like cluttering. I changed a few occurrences, but would not want to make it a rule. Anyway, there are all these 'sb', 'is', 'bais', etc. all over the codebases. It is true though, that 'anno' might be misunderstood as year.

Setting this to merge ready as I only made very minor changes. Please check again before merging.

comment:9 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Tweaked the change log a bit, rebased to master, and pushed.

Regarding variable names, I think we shouldn't use abbreviations other than commonly known ones like IP or HTTP etc. It's true that there are quite a few sb and is and so on in the code. We might replace them with more meaningful names.

Closing. Thanks!

Note: See TracTickets for help on using tickets.