You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/25 13:44:01 UTC
[GitHub] [kafka] dongjinleekr opened a new pull request #10402: TRIVIAL: Minor improvements
dongjinleekr opened a new pull request #10402:
URL: https://github.com/apache/kafka/pull/10402
1. Fix typo: clustes → clusters
2. Remove unused class: `RemoteClusterUtils`
3. Remove unthrown Exceptions: `MirrorClientTest`, `MirrorHeartbeatTask`, and `MirrorSourceConnector`.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-807925585
Retest this please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10402: TRIVIAL: Minor improvements
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#discussion_r601774493
##########
File path: connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/RemoteClusterUtils.java
##########
@@ -1,94 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.kafka.connect.mirror;
-
-import org.apache.kafka.common.TopicPartition;
-import org.apache.kafka.clients.consumer.OffsetAndMetadata;
-
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeoutException;
-import java.time.Duration;
-
-
-/** Convenience methods for multi-cluster environments. Wraps MirrorClient (@see MirrorClient).
- * <p>
- * Properties passed to these methods are used to construct internal Admin and Consumer clients.
- * Sub-configs like "admin.xyz" are also supported. For example:
- * </p>
- * <pre>
- * bootstrap.servers = host1:9092
- * consumer.client.id = mm2-client
- * </pre>
- * <p>
- * @see MirrorClientConfig for additional properties used by the internal MirrorClient.
- * </p>
- */
-public final class RemoteClusterUtils {
Review comment:
Oh thanks, didn't notice it was public. Glad I asked
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ryannedolan commented on a change in pull request #10402: TRIVIAL: Minor improvements
Posted by GitBox <gi...@apache.org>.
ryannedolan commented on a change in pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#discussion_r601772795
##########
File path: connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/RemoteClusterUtils.java
##########
@@ -1,94 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.kafka.connect.mirror;
-
-import org.apache.kafka.common.TopicPartition;
-import org.apache.kafka.clients.consumer.OffsetAndMetadata;
-
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeoutException;
-import java.time.Duration;
-
-
-/** Convenience methods for multi-cluster environments. Wraps MirrorClient (@see MirrorClient).
- * <p>
- * Properties passed to these methods are used to construct internal Admin and Consumer clients.
- * Sub-configs like "admin.xyz" are also supported. For example:
- * </p>
- * <pre>
- * bootstrap.servers = host1:9092
- * consumer.client.id = mm2-client
- * </pre>
- * <p>
- * @see MirrorClientConfig for additional properties used by the internal MirrorClient.
- * </p>
- */
-public final class RemoteClusterUtils {
Review comment:
This is a public interface and js definitely being used in external projects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-807859296
@ableegoldman Here it is. I removed one commit and changed the title.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10402:
URL: https://github.com/apache/kafka/pull/10402
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-823825975
@chia7712 🙇🙇🙇
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-822992332
@chia7712 This PR seems already approved but not merged yet. Could you have a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-822997580
> This PR seems already approved but not merged yet. Could you have a look?
oh, sorry for neglecting this PR :(
Will commit it if QA pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr removed a comment on pull request #10402: MINOR: Remove unthrown exceptions, fix typo, etc.
Posted by GitBox <gi...@apache.org>.
dongjinleekr removed a comment on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-807925585
Retest this please.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #10402: TRIVIAL: Minor improvements
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#discussion_r601946043
##########
File path: connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/RemoteClusterUtils.java
##########
@@ -1,94 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.kafka.connect.mirror;
-
-import org.apache.kafka.common.TopicPartition;
-import org.apache.kafka.clients.consumer.OffsetAndMetadata;
-
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeoutException;
-import java.time.Duration;
-
-
-/** Convenience methods for multi-cluster environments. Wraps MirrorClient (@see MirrorClient).
- * <p>
- * Properties passed to these methods are used to construct internal Admin and Consumer clients.
- * Sub-configs like "admin.xyz" are also supported. For example:
- * </p>
- * <pre>
- * bootstrap.servers = host1:9092
- * consumer.client.id = mm2-client
- * </pre>
- * <p>
- * @see MirrorClientConfig for additional properties used by the internal MirrorClient.
- * </p>
- */
-public final class RemoteClusterUtils {
Review comment:
Oh, I see. I will revert it. Thanks @ryannedolan! :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #10402: TRIVIAL: Minor improvements
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#discussion_r601767570
##########
File path: connect/mirror-client/src/main/java/org/apache/kafka/connect/mirror/RemoteClusterUtils.java
##########
@@ -1,94 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.kafka.connect.mirror;
-
-import org.apache.kafka.common.TopicPartition;
-import org.apache.kafka.clients.consumer.OffsetAndMetadata;
-
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeoutException;
-import java.time.Duration;
-
-
-/** Convenience methods for multi-cluster environments. Wraps MirrorClient (@see MirrorClient).
- * <p>
- * Properties passed to these methods are used to construct internal Admin and Consumer clients.
- * Sub-configs like "admin.xyz" are also supported. For example:
- * </p>
- * <pre>
- * bootstrap.servers = host1:9092
- * consumer.client.id = mm2-client
- * </pre>
- * <p>
- * @see MirrorClientConfig for additional properties used by the internal MirrorClient.
- * </p>
- */
-public final class RemoteClusterUtils {
Review comment:
I see this isn't currently being used, but we should be careful about removing it completely. This might just be the groundwork for additional testing which hasn't been completed yet. Seems like this class was added as part of the MirrorMaker2 work, which still has at least one open [PR for system tests](https://issues.apache.org/jira/browse/KAFKA-8929) which might rely on this utility class.
@ryannedolan can you confirm whether this class is definitively no longer needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #10402: TRIVIAL: Minor improvements
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on pull request #10402:
URL: https://github.com/apache/kafka/pull/10402#issuecomment-806770769
@dajac @ableegoldman Could you have a look? I found these when I was working on [KAFKA-10585](https://issues.apache.org/jira/browse/KAFKA-10585).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org