Matthew, I stole your integration tests to prove that point, but I would like to make sure you get credit for them in the commit history. If you want to break out your tests into a separate commit, I can cherry-pick them into my PR.
That would yield a PR with: - a couple bug fixes from me in MirrorSourceConnector and MirrorClient in order to support IdentityReplicationPolicy - my IdentityReplicationPolicy, based on yours and previous work, but without the API change - your tests ...which I think we shouldn't have trouble getting merged. We still have the open question of whether to call this IdentityReplicationPolicy (which seems to be the general consensus) or LegacyReplicationPolicy (which is what I called it back in KIP-382). I'm onboard with IdentityReplicationPolicy, but this [Discuss] thread is a good place to bring it up in case anyone objects. Ryanne On Thu, May 20, 2021 at 5:22 PM Matthew de Detrich <matthew.dedetr...@aiven.io.invalid> wrote: > @Ryanne, > > I just noted you updated your PR at > > https://github.com/apache/kafka/pull/10652/files#diff-79a09517576a35906123533490ed39c0e1a9416878e284d7b71f5f4c53eeca29R31 > and I was mistaken in regards to the API changes being required. In this > case we can just use your PR instead of mine without the need for a KIP. > > On Wed, May 19, 2021 at 11:12 AM Matthew de Detrich < > matthew.dedetr...@aiven.io> wrote: > > > Hey Ryanne, > > > > Thanks for the reply, personally I have a slight preference for my > > implementation since it doesn't require the "cheating" with the > > remote.topic.suffix as you mentioned (this also makes my implementation a > > bit more clean/simple) but I am definitely not opposed to adjusting to > use > > your method in order to avoid needing KIP (however as you stated in order > > to get all usecases we would have to create a KIP later anyways). > > > > What are your thoughts on seeing how the KIP works out, and if it takes > > too long or there is some issue with it we can go along with your > > implementation as an initial step? Though this is my first time doing a > KIP > > so I am not sure how long it typically takes to get one approved. > > > > Regards > > > > On Wed, May 19, 2021 at 3:58 AM Ryanne Dolan <ryannedo...@gmail.com> > > wrote: > > > >> Hey Matthew, as you call out in the KIP there are few impls floating > >> around, including my WIP PR here: > >> > >> https://github.com/apache/kafka/pull/10652 > >> > >> The tests are currently passing except for a couple asserts related to > >> failback (commented out). It appears your PR doesn't address failback, > so > >> I > >> think we can consider the two impls functionally equivalent, more or > less. > >> > >> However, I've cheated a bit here: I've added a "remote.topic.suffix" > >> property and have the integration tests configured to use it. So I can > get > >> active/active replication to _mostly_ work with my impl, but that's not > >> really a requirement per se. It's fine with me if > >> Identity/LegacyReplicationPolicy explicitly only supports > active/passive. > >> In that case, we can drop the "remote.topic.suffix" stuff, which would > >> require a separate KIP anyway. > >> > >> I think that means we can take my ReplicationPolicy (minus > >> remote.topic.suffix) and your tests and get to a working state without > any > >> changes to the public interface, wdyt? > >> > >> Ryanne > >> > >> On Mon, May 10, 2021 at 6:02 PM Matthew de Detrich > >> <matthew.dedetr...@aiven.io.invalid> wrote: > >> > >> > Hello everyone. > >> > > >> > I have a KIP that involves adding a public method to the > >> ReplicationPolicy > >> > interface called canTrackSource. The intention behind creating this > >> method > >> > is to implement an IdentityReplicationPolicy (aka > >> LegacyReplicationPolicy) > >> > which is a MirrorMaker2 ReplicationPolicy that behaves the same way as > >> the > >> > original MirrorMaker1 ReplicationPolicy. There is already a passing > >> > implementation at https://github.com/apache/kafka/pull/10648. > >> > > >> > It would be ideal for this change (if approved) to land for Kafka > 3.0.0 > >> > since there is a change to a public interface but do not there aren't > >> any > >> > breaking changes. > >> > > >> > > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-737%3A+Add+canTrackSource+to+ReplicationPolicy > >> > > >> > Cheers > >> > > >> > -- > >> > > >> > Matthew de Detrich > >> > > >> > *Aiven Deutschland GmbH* > >> > > >> > Immanuelkirchstraße 26, 10405 Berlin > >> > > >> > Amtsgericht Charlottenburg, HRB 209739 B > >> > > >> > *m:* +491603708037 > >> > > >> > *w:* aiven.io *e:* matthew.dedetr...@aiven.io > >> > > >> > > > > > > -- > > > > Matthew de Detrich > > > > *Aiven Deutschland GmbH* > > > > Immanuelkirchstraße 26, 10405 Berlin > > > > Amtsgericht Charlottenburg, HRB 209739 B > > > > *m:* +491603708037 > > > > *w:* aiven.io *e:* matthew.dedetr...@aiven.io > > > > > -- > > Matthew de Detrich > > *Aiven Deutschland GmbH* > > Immanuelkirchstraße 26, 10405 Berlin > > Amtsgericht Charlottenburg, HRB 209739 B > > *m:* +491603708037 > > *w:* aiven.io *e:* matthew.dedetr...@aiven.io >
