Add additional timestamp sources

[mkeskells]

from a header value
from a data field
via a custom extractors

[Claudenw]

Sorry it took so long to get back to this review.

[Claudenw]

This method does not make sens to me.

There should be a setType(Type type) method and a setArgs(String... args) method. Worry about configuration packing outside of this class. I know that you are thinking that you can put HEADER:field into a configuration but do the unpacking outside of this class, then you don't have to jump through hoops to change the split pattern or similar gyrations.

[mkeskells]

happy to separate the type and parameter, but its not varargs

[mkeskells]

I think its probably better to allow all of the additional class parameters to be able to be specified via additional parameters - which become available with #300. That way its explicit, documented and extensible

[Claudenw]

There are no checks for any of these being null.

[mkeskells]

There doesn't need to be. The caller validates. Adding null checks at all levels just blurs the concerns IMO

[Claudenw]

If you make this an abstract method then you can simply override it in your implementations and not pollute this class with methods that are only used by one instance. This will also make the code easier to read and maintain.

[mkeskells]

This allows reuse. Its can still be overridden if a subclass needs it but doesn't require that. What is here is a default, available to be

[mkeskells]

there are two separate concerns here. The location of the date like field, and the conversion of that to a ZonedDateTime - e.g. 982b347 adds java.util.Date as a source conversion
This isn't polluting the api, its promoting reuse, and when extending this library to allow external code to use it, I believe its best to enable extension code to access to common and shared concepts, like implicit type conversion

[Claudenw]

Why not a class that just does the conversions. I think that classes should try to do one thing and do it very well. A class to convert time formats means they are reusable elsewhere in the chain.

[mkeskells]

so you want o have the Extraction as one component in the TimstampSource?
So roughly an extractor and a convertor?

that not the way that the existing TimestampSources work

e.g.
for event

        @Override
        public ZonedDateTime time(final SinkRecord record) {
            return ZonedDateTime.ofInstant(Instant.ofEpochMilli(record.timestamp()), zoneId);
        }

does 3 things locate, convert and add zoneId

Do you want to change the split of the functionality for all of these existing cases? or have I missed what you are proposing/unhappy about

[Claudenw]

The constructor argument seems backwards. ZoneId is required for all instances so shouldn't that be first and then the params?

[mkeskells]

by definition both are required parameters in the signature

[mkeskells]

I think that its probably better to move all of the configuration to separate properties, once #300 is merged. That allows for external classes to add to the configuration parameters, and read them. That way its not limited in the configuration, and self documenting

Thoughts?