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
