Opened 9 months ago

Closed 7 months ago

#33090 closed enhancement (fixed)

Make all Descriptor implementors serializable

Reported by: notirl Owned by: metrics-team
Priority: High Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points: 0.5
Parent ID: Points:
Reviewer: irl Sponsor:

Description

For most distributed/cloud data frameworks, it is necessary to serialize objects to pass them around the system.

This change should be as simple as having the Descriptor interface extend Serializable, and then adding a constant serial UID to every implementing class.

As we currently don't declare these as serializable, nothing is trying to serialize them, so this should be a low-risk change.

The only issue I can see happening is if there are any members of implementation classes that are not serializable, but the majority of things are afaict.

Child Tickets

Change History (6)

comment:1 Changed 9 months ago by karsten

Can you try this out in a branch and use a jar file produced by that for the distributed/cloud data frameworks you have in mind?

I'd also think that most classes are serializable. The issue is that by adding the Serializable interface we are declaring all descriptors as serializable in the API. We should only do that if we are confident that it will very likely not break anything. Maybe your test can give us that confidence?

comment:2 Changed 9 months ago by notirl

It looks like it needs to be not just the Descriptor interface, because some other interfaces are not subinterfaces of that. We probably would want to make all our interfaces serializable in metrics-lib.

The only issue I have found is where we use TreeMap because the comparator in there is an anonymous class that is not serializable. We can override those comparators with serializable but still anonymous comparators instead.

I'm able to have consensus objects passed around in Apache Beam now, and pull out the nickname of every relay in a consensus last month at record speed.

comment:3 Changed 7 months ago by karsten

Cc: metrics-team added
Priority: MediumHigh
Reviewer: irl
Status: newneeds_review

Alright, I have a branch that I tested with CollecTor's recent/ directory and which might also work for your use case. (The trick with that TreeMap was to turn the lambda comparator into its own serializable Comparator class. It's not that all TreeMaps are non-serializable, AFAIK.)

Can you give this branch a try? The patch is commit e2fc2cb in my task-33090 branch.

We should soon decide about revising/merging this branch, because it has high potential of creating merge conflicts with all the files it touches. Setting priority to high for that reason.

comment:4 Changed 7 months ago by irl

Status: needs_reviewmerge_ready

This looks good to me, and Apache Beam is happy to parse consensuses in pipelines now and hand back the object.

comment:5 Changed 7 months ago by karsten

Actual Points: 0.5

Great, thanks for reviewing and testing this patch! Merged to master.

I'm putting in my 0.5 actual points for working on this patch. Please add your actual points, so that we get a realistic picture how much effort we spent on this enhancement. And please close the ticket afterwards.

comment:6 in reply to:  5 Changed 7 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Replying to karsten:

I'm putting in my 0.5 actual points for working on this patch. Please add your actual points, so that we get a realistic picture how much effort we spent on this enhancement. And please close the ticket afterwards.

As discussed last Thursday, we shouldn't keep tickets open just for actual points not being updated yet. We can update tickets for that even after they're closed. Closing.

Note: See TracTickets for help on using tickets.