Opened 3 years ago

Closed 3 years ago

#26162 closed enhancement (fixed)

Replace Gson with Jackson in CollecTor

Reported by: karsten Owned by: karsten
Priority: High Milestone: CollecTor 1.6.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


Related to Onionoo ticket #25848 and metrics-lib ticket #26159, we should replace Gson with Jackson in CollecTor. For reasons, see those tickets. I'll work on this.

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by karsten

Status: assignedneeds_review

comment:2 Changed 3 years ago by iwakeh

Status: needs_reviewneeds_revision

CollecTor already uses all classes. Why duplicate the writing functionality? Maybe, this should be implemented by using only classes in CollecTor? Future json processing lib changes would not affect CollecTor anymore (except for simply including the dependency).

Setting to needs_revision for discussion.

comment:3 Changed 3 years ago by karsten

Let's only do this if the changes are really small. It would be really bad to spend too much time on CollecTor so that we won't have enough time left for the other releases.

Do you want to write a patch that does what you suggest, and I continue preparing and testing the other patches? And if it turns out to be harder than expected, we save this additional step for later?

(By the way, CollecTor also uses a JSON formatter/parser for the reference checker in its relay descriptors module. That means, even if we use metrics-lib classes for index.json formatting and parsing, we'll still have unrelated JSON code in CollecTor.)

comment:4 Changed 3 years ago by iwakeh

Status: needs_revisionneeds_review

Please see this commit removing jackson dependencies from index-module.

Reference checker is ok. All checks&tests pass.

comment:5 Changed 3 years ago by karsten

Status: needs_reviewmerge_ready

Heh, alright, those changes are indeed really small. Merged to my branch. If you don't mind, I'll squash that commit into mine for the release, for a clearer Git history. Setting to merge_ready and preparing a pre-release tarball later today. Thanks!

comment:6 Changed 3 years ago by karsten

Status: merge_readyneeds_review

I just pushed fixup commit 01f6329 to my task-21612 branch that fixes an issue in the reference checker: while it's usually a good idea to skip fields with null or empty values, say, in Onionoo, the reference checker cannot handle that and needs to serialize and de-serialize empty strings. Before this fixup commit it would have de-serialized empty strings as null values, and that would have broken comparisons. Please review!

comment:7 Changed 3 years ago by iwakeh

The fix looks fine.
To prevent future changes that might undo this unusual (in metrics products at least) behavior could you add a tiny test to ReferenceCheckerTest?

comment:9 Changed 3 years ago by iwakeh

Status: needs_reviewmerge_ready

Cool! All fine.

comment:10 Changed 3 years ago by karsten

Milestone: CollecTor 1.6.0
Resolution: fixed
Status: merge_readyclosed

This is released as part of CollecTor 1.6.0. Closing. Thanks!

Note: See TracTickets for help on using tickets.