Thanks for this, I wasn't aware of the co-authored capabilities for git/github. In this case @Ryanne I don't think I need to split apart my commits, you can just add my email/name to the commit. Also make sure to add ivanyu (https://github.com/ivanyu) since he made the original PR/tests at https://github.com/apache/kafka/pull/9395, I just fixed/rebased them for the latest trunk.
On Fri, May 21, 2021 at 12:48 PM Mickael Maison <mickael.mai...@gmail.com> wrote: > When merging, all commits will be squashed. > However, a committer can set "Co-authored-by" tags in the commit > message to credit all authors. Just make sure it's clearly stated in > the PR, so the committer is aware of it. > > > https://docs.github.com/en/github/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors > > On Fri, May 21, 2021 at 10:00 AM Matthew de Detrich > <matthew.dedetr...@aiven.io.invalid> wrote: > > > > 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 > -- Matthew de Detrich *Aiven Deutschland GmbH* Immanuelkirchstraße 26, 10405 Berlin Amtsgericht Charlottenburg, HRB 209739 B Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen *m:* +491603708037 *w:* aiven.io *e:* matthew.dedetr...@aiven.io
