KAFKA-3522: Generalize Segments

[mjsax]

Part of KIP-258: In order to support old and new segments (ie, without timestamps and with timestamps), the code is restructured:

• splitting Segment into interface Segment and KeyValueSegment class
• splitting Segments into AbstractSegments and KeyValueSegments

This is no functional change. This is also completely internal and we can merge before the KIP is accepted.

[mjsax]

Add this as dummy because I reference it in JavaDocs of AbstractSegments -- will be implemented in follow up PR.

[vvcephei]

Not a big deal, but I'm not a huge fan of dead code (here or below). IMHO, it would have been better to avoid adding the reference in the javadoc and thereby avoid the obligation to add both of these un-implemented, un-tested, and un-used classes.

No change requested, just sharing my thoughts ;) I know you're queuing up the implementations in another PR right away.

[mjsax]

While I agree, I did it this way, because otherwise, updating the Javadoc would have most likely slipped...

[mjsax]

Introduce generic Segment type so we can use it for both segment types

[mjsax]

This must be implement in derived classed now

[mjsax]

This code is copied from old class Segment

[mjsax]

This replaces old class Segments

[mjsax]

Now an interface -- actual code moved to KeyValueSegment

[mjsax]

Add this as dummy because I reference it in JavaDocs of AbstractSegments -- will be implemented in follow up PR.

[mjsax]

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman

[bbejeck]

Thanks for this @mjsax, this LGTM just one minor comment.

[bbejeck]

nit: I'm wondering if this would be easier to read

if(segments.put(segmentId, newSegment)!= null){
  throw new IllegalStateException("KeyValueSegment already exists. Possible concurrent access.");
}

However, this is highly opinated

[mjsax]

Updated per comment

[vvcephei]

Looks good overall, @mjsax . I left a few comments. I'm really just concerned about the access to the previously-private fields and the name of AbstractSegments.

Thanks!

[vvcephei]

Thanks to the flat package structure, this change actually exposes these fields to 66 other classes, all of our other internal state-related code. This is actually pretty risky, since segments in particular is mutable.

Your intent is to broaden the scope to only the subclasses, but Java has no visibility modifier to do that. (protected seems to be the one, but its visibility is actually broader than package-protected)

To enforce the desired visibility restriction, we would have to create a new package, org.apache.kafka.streams.state.internals.segments, and locate the abstract class and both implementations in it. Then the fields would be visible only to the implementing classes, as desired.

Personally, I think the mutable segments field justifies this, as future internal state-store code changes might accidentally think it's ok to touch segments, directly, leading to subtle bugs.

[mjsax]

I don't see this danger. Note, that all changes must be approved by a committer and thus, I am sure this would be caught (it's not a project for which somebody will limited knowledge can easily push changes). If you insist on a different package structure I am also fine with it, even if I don't see a bid advantage introducing many small packages.

\cc @guozhangwang

[vvcephei]

In general, there may not be a big advantage to many small packages, but in this particular case, it happens to be the only way to safely protect what is intended to be private state.

Personally, I wouldn't lean too heavily on the eternal vigilence of reviewers to catch subtle bugs when there's an easy way to prevent them structurally. I certainly wouldn't trust myself to just remember that this field, although it's made accessible to the whole package, is actually supposed to be used only by subclasses. In two years' time, I think I would just assume it's supposed to be accessible, since that's what the code says.

Also curious what @guozhangwang and @bbejeck think.

[vvcephei]

Ah, I see you already added the interface. This is fine, too!

[guozhangwang]

The current hierarchy looks good to me.

[vvcephei]

Not a big deal, but I'm not a huge fan of dead code (here or below). IMHO, it would have been better to avoid adding the reference in the javadoc and thereby avoid the obligation to add both of these un-implemented, un-tested, and un-used classes.

No change requested, just sharing my thoughts ;) I know you're queuing up the implementations in another PR right away.

[vvcephei]

It's unusual to hold a reference to an abstract class like this. I believe the intent is to be able to transparently handle either KeyValueSegments or (I'm guessing) KeyValueTimestampSegments.

The full expression would be to have a Segments interface implemented by AbstractSegments, which is then extended by your two implementations. Then this line would reference Segments<S>. It's fine to collapse this into just the abstract class (although questionable in the presence of package-protected fields). But to maintain transparency, I'd name the abstract class Segments instead of AbstractSegments. That way, to an outsider class (like this one), you're still just interacting with the interface (i.e., the public interface of the class), rather than specifically an abstract class.

Adhering to this pattern leaves the door open in the future to extract Segments into a full interface without having to change any outside code (which is what I meant by maintain transparency).

[mjsax]

It's unusual to hold a reference to an abstract class like this. I believe the intent is to be able to transparently handle either KeyValueSegments or (I'm guessing) KeyValueTimestampSegments.

Correct.

I don't see a big improvement adding an interface, but if it makes you happy :)

[vvcephei]

I agree, and it might by splitting hairs, but I actually was only suggesting to rename AbstractSegments to Segments. Although some projects require all references to be interfaces, it generally leads to low-value bloat. I think it's fine to reference classes, since all classes already present a public "interface".

I was suggesting to rename the class so that the reference here doesn't declare that its target is an abstract class, which is irrelevant. Renaming it Segments frees us up to extract Segments as an interface in the future if we wish.

[vvcephei]

It's an antipattern for a superclass to specify what subclasses it, specifically because it's likely that the next time someone adds a new subclass, it's unlikely they'll remember to update this comment, so then this comment will become misleading.

Plus, the HTML doc generator will automatically create a list of known subclasses, and any IDE also makes it trivially easy to discover what implements it.

Bonus: removing these references frees you from the obligation to add the dead-code classes that they point to.

IMHO, all the javadoc really needs to say is that this class manages {@link Segment}s.

[mjsax]

Ack. Will remove the two dead code classes and remove this comment.

[mjsax]

Updated this.

[guozhangwang]

Just a minor question about the template on key schema, feel free to merge after replied.

[guozhangwang]

Is Session/WindowKeySchema always templated on KeyValueSegment (seems indicated in the javadoc below at line 157)?

[mjsax]

Yes. Atm, KeyValueSegment is the only class implementing Segment.

[mjsax]

As discussed in person: we can simplify this and remove the generic type from the interface and only use a generic type on method segmentsToSearch.

[guozhangwang]

The current hierarchy looks good to me.

[mjsax]

Rebased this to resolve merge conflicts. Merging this after Jenkins passed.

[mjsax]

Updated this to simplify generics.

[guozhangwang]

LGTM! Please feel free to merge after tests passed.

[mjsax]

Unrelated tests failed. Retest this please.