Opened 2 months ago

Last modified 4 days ago

#33090 merge_ready enhancement

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:


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 (5)

comment:1 Changed 2 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 2 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 3 weeks 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 5 days 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 4 days 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.

Note: See TracTickets for help on using tickets.