fix: Make InitialOffset available also in PartitionConsumer

[fredde-fisk]

Store the initial offset in the partitionConsumer
and make it available via the InitialOffset
method. The same method is then also reused when
embedded in a consumerGroupClaim.

I also added tests for the InitialOffset method since it did not exist before.

This change will align the ConsumerGroupClaim and PartitionConsumer interfaces, and will also make it possible to implement a proper end of partition detection that is not timer based or has to access internal fields, something that has been requested several times (#628, #848, #2116).

[puellanivis]

Contains a breaking change 🤔 is this unavoidable?

[puellanivis]

As we’ve pointed out in other PRs, adding new methods to an interface is technically a breaking change. 🤔

[fredde-fisk]

I couldn't find how/if you have solved similar things before, so I don't know if you have a general strategy, but here are some alternatives that I see:

1. Add helper function that returns the partitionConsumer.initialOffset, for example:

   func PartitionConsumerInitialOffset(pc PartitionConsumer) int64 {
       if p, ok := pc.(*partitionConsumer); ok {
           return p.initialOffset
       }
   
       return -1 // or panic
   }
   

   This will however clutted the package namespace.

2. Keep partitionConsumer.InitialOffset() but not in the PartitionConsumer interface. To access the method the user has to define a new interface with InitialOffset(), for example:

   pc.(interface{ InitialOffset() int64 }).InitialOffset()
   

   This will however hide the method from the actual public API, making it semi-internal.

3. Add a new interface with InitialOffset() available under a new name (e.g. with a version number). For example:

   type PartitionConsumerV2 interface {
       PartitionConsumer
       InitialOffset()
   }
   
   // To use the new `PartitionConsumerV2` interface
   pc := consumer.ConsumePartition(...).(sarama.PartitionConsumerV2)
   // or through a new set of constructors
   pc := consumer.ConsumePartitionV2(...)
   
   pc.InitialOffset()
   

   This will however become a bit messy if there are a lot of new versioned interfaces. At a major release, when breaking backward compatibility, the versioned interfaces can be flattened to get a single interface again.

[puellanivis]

Indeed, this basically why there’s the general guideline “return concrete types, and accept interfaces”. If the PartitionConsumer were actually the unexported partitionConsumer type, then we could easily expand the receiver methods all we want. But we’re stuck at this version with what we have.

Given the lock in, the preferred way to expand an interface is with an expansion interface example:

type InitialOffsetPartitionConsumer interface{
	// N.B. Do not use "V2", instead be descriptive about what's being added.
	PartitionConsumer
	InitialOffset() int64
}

As a good stdlib example see: fs.ReadDirFS, an expansion interface on ̀ fs.FSfor things that also implement aReadDir` method. This would mostly align with your option 2, but there’s an important distinction: each extension should supply the minimal amount of expansion for that particular feature, and each extension should build separately off of the base interface†, and not just keep aggregating up more and more interfaces.

†: unless it depends on an earlier feature, and thus would need to be an extension interface on that interface.

This will however become a bit messy if there are a lot of new versioned interfaces.

Yes, this can build up quite a lot of extension interfaces, and that can get a little messy. But this is also basically the best way to provide selective feature implementation on interfaces.

At a major release, when breaking backward compatibility, the versioned interfaces can be flattened to get a single interface again.

No, no capes interfaces. On a major release, any library that is returning interfaces with unexported implementations should switch to returning exported concrete types, which can expand their receiver method coverage at will without it being a breaking change.

[fredde-fisk]

I moved InitialOffset to a InitialOffsetPartitionConsumer interface. I also changed the internal consumerGroupClaim.PartitionConsumer from an interface to the concrete type that it will actually contain, which will then handle the implementation of InitialOffset in the ConsumerGroupClaim interface as well.

[puellanivis]

Usually embedded types are placed at the top of a type.

[fredde-fisk]

Moved to the top

[puellanivis]

I would adjust the language here that we’re returning an “untyped nil” and “rather than… a typed pointer nil”. (The disambiguation of these two conflated terms being the whole point of the comment here.)

[fredde-fisk]

So "Return untyped nil rather than a typed pointer nil"?

[puellanivis]

Sorry, the additional context you have/had is great. To be more clear: Return untyped nil rather than a PartitionConsumer interface with a typed nil pointer. is great.

(Now that I type it out, and read it, “typed nil pointer” sounds better than “typed pointer nil”.)

[fredde-fisk]

Changed the wording in the comments