[Conjure Java Runtime] Part 17: 🚨🚚🚨 Optionals for Paxos Learners

[jeremyk-91]

Goals (and why):

• Fulfil a prerequisite for internode communication via Conjure Java Runtime

Implementation Description (bullets):

• Switch PaxosLearner to return Optional<PaxosValue> and fix attendant breaks.
• Also switched TimelockPaxosLearnerRpcClient to use this.

Testing (What was existing testing like? What have you done to improve it?):

• Updated tests that used to use nullable values to now use Optionals. These passing should be sufficient.

Concerns (what feedback would you like?):

• This is obviously an API break, is it unforgivably large?
• There is no wire break (a wire break would obviously be disastrous) - please confirm this.

Where should we start reviewing?: PaxosLearner

Priority (whenever / two weeks / yesterday): this week please

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

The PaxosLearner interface now returns Optional<PaxosValue> instead of a nullable PaxosValue for the getLearnedValue(seq) and getGreatestLearnedValue() methods. The wire format remains unchanged (so, for example, rolling upgrades of TimeLock, or servers in an embedded leader configuration, are safe). Users who require the old behaviour can use Optional.orElse(null).

Check the box to generate changelog(s)

• Generate changelog entry

[OStevan]

Call to isPresent can be removed, hasValueSatisfying will also assert that the value is present.

[jeremyk-91]

you're right! Will change this

[felixdesouza]

can we go further:

Optional<PaxosValue> value = learner.getGreatestLearnedValue();
assertThat(value).map(PaxosValue::getLeaderUUID).contains(PAXOS_UUID);
assertThat(value).map(PaxosValue::getData).contains(PAXOS_DATA);

[felixdesouza]

note, map is a method on the OptionalAssert

[jeremyk-91]

(yep, changed to OptionalAssert's map)

[OStevan]

If I understood correctly that empty result is a "non-standard" path I would invert the if.

[felixdesouza]

unrelated to above: this shouldn't be null right? since the map is not empty, unless you're thinking it has null values, but non-null keys

[jeremyk-91]

Agree on inverting if (didn't as I just translated what was there, but yeah it does make sense).

I wrote Optional.of() first, but there were some tests that failed. Possibly a bad test, I didn't investigate in detail

[felixdesouza]

Sadness is sad

[felixdesouza]

💃

[felixdesouza]

Don't think we need the @Nonnull annotations, since that should be implied with code wide Nonnull by default and also the fact that it's a huge code smell to pass around a null Optional

[jeremyk-91]

This was done for consistency with the other methods in the client. I guess now all of them are nonnull it's probably fine to remove them.

[jeremyk-91]

Thanks!