Sure thing, i will split out my PR into 2 commits by EOD today so that you can cherry pick the test commit in your PR.
Thanks a lot for the work! On Fri, May 21, 2021 at 12:53 AM Ryanne Dolan <ryannedo...@gmail.com> wrote: > 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 > > > -- 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
