Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
omkreddy merged PR #15048: URL: https://github.com/apache/kafka/pull/15048 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
stanislavkozlovski commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880919489 Let's merge to 3.7 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
omkreddy commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880911951 > @omkreddy Does this work for you? > > 1. Checkout the branch from the PR and go to the `docker` directory > 2. Run `python3 docker_build_test.py kafka/test --image-tag=3.6.0 --image-type=jvm --kafka-url=https://archive.apache.org/dist/kafka/3.6.0/kafka_2.13-3.6.0.tgz` Hi @mimaison, It will not work now, as the newly added `KafkaDockerWrapper.scala` classes are not part of 3.6. For now, I have tested with custom image and few changes to the PR. We can use above cmd once we have 3.7 RC0. As suggested by you, we need docker build option for PRs. I have requested the same from @VedarthConfluent. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880864298 +1 for cherry-picking on 3.7 if Stanislav agrees -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880862505 @omkreddy Does this work for you? 1. Checkout the branch from the PR and go to the `docker` directory 2. Run `python3 docker_build_test.py kafka/test --image-tag=3.6.0 --image-type=jvm --kafka-url=https://archive.apache.org/dist/kafka/3.6.0/kafka_2.13-3.6.0.tgz` -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880861940 I have created a jira to track the new github actions pipeline requested above https://issues.apache.org/jira/browse/KAFKA-16091 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1444502244
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
Done! https://issues.apache.org/jira/browse/KAFKA-16090
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
omkreddy commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1880849914 > I saw that but the action only allows me to pick a branch or tag. Ideally I'd like to run it on the pull request. @VedarthConfluent Can you pls crate JIRA to track this? -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
omkreddy commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r162085
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
Can we create JIRA to track this?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1443686810
##
docker/jvm/launch:
##
@@ -28,25 +27,25 @@ fi
# the default is to pick the first IP (or network).
export KAFKA_JMX_HOSTNAME=${KAFKA_JMX_HOSTNAME:-$(hostname -i | cut -d" " -f1)}
-if [ "$KAFKA_JMX_PORT" ]; then
+if [ "${KAFKA_JMX_PORT-}" ]; then
# This ensures that the "if" section for JMX_PORT in kafka launch script
does not trigger.
export JMX_PORT=$KAFKA_JMX_PORT
-export KAFKA_JMX_OPTS="$KAFKA_JMX_OPTS
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
+export KAFKA_JMX_OPTS="${KAFKA_JMX_OPTS-}
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
fi
# Make a temp env variable to store user provided performance otps
-if [ -z "$KAFKA_JVM_PERFORMANCE_OPTS" ]; then
+if [ -z "${KAFKA_JVM_PERFORMANCE_OPTS-}" ]; then
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS=""
else
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS"
fi
# We will first use CDS for storage to format storage
-export KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
+export KAFKA_JVM_PERFORMANCE_OPTS="${KAFKA_JVM_PERFORMANCE_OPTS-}
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
echo "===> Using provided cluster id $CLUSTER_ID ..."
# A bit of a hack to not error out if the storage is already formatted. Need
storage-tool to support this
Review Comment:
Update on this, I have moved this check back to the bash script, given that
the documentation suggests that `--ignored-formatted` flag should only be used
in a very specific scenario only and I couldn't find another way to handle it
in the wrapper. Maybe it will be feasible once storage tool is rewritten.
Given that this refactor was mentioned as a minor concern, hopefully it
should be okay if we keep this small logic in bash script for now.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
viktorsomogyi commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1442969642
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
I'm OK with that (refactoring later).
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
viktorsomogyi commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1442969642
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
I'm OK with that.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1442692990
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
`StorageTool` is being rewritten in Java in
https://github.com/apache/kafka/pull/14847 so we can probably keep the code as
is for now and see if we can tidy things up once the rewrite PR is merged.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1442527460
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
The way storage tool is written, we will need the logic that's present in
the main function. We can create a new function that contains entirety of the
main function logic and call that from both storage tool main and our code. But
that will be kind of same as the present solution, but with just one extra
method.
Proper solution will require thorough refactoring of the storage tool, which
I think is out of scope for this change. Please let us know your thoughts on
this.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1442527460
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
The way storage tool is written, we will need the logic that's present in
the main function. We can create a new function hat contains entirety of the
main function logic and call that from both storage tool main and our code. But
that will be kind of same as the present solution, but with just one extra
method.
Proper solution will require thorough refactoring of the storage tool, which
I think is out of scope for this change. Please let us know your thoughts on
this.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
kagarwal06 commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1877080323 > I saw that but the action only allows me to pick a branch or tag. Ideally I'd like to run it on the pull request. Hi @mimaison , thanks for the feedback. The dockerisation of Apache Kafka follows the following process - Accepts a signed Kafka tarball link. - Expects the presence of both the `.asc` file and `key` for tarball verification. This verification process is implemented in the [Dockerfile](https://github.com/apache/kafka/blob/trunk/docker/jvm/Dockerfile#L78-L83). The `docker_build_test.py`, utilized in GitHub Actions as well, takes an Apache Kafka tarball URL, supplies it to the Dockerfile, builds the Docker image, and runs tests on it. This approach is valuable during Release Candidate (RC) testing, allowing us to verify the Docker image by providing the Apache Kafka RC link along with the necessary `.asc` file and the `key`. However, in the context of pull requests, especially when changes impact the Apache Kafka codebase and the docker path (as in the current PR), testing requires a signed tarball with the associated `keys` and `.asc` file. Presently, our pipeline is not designed for this use-case. **NOTE:** We have locally tested the above changes by building the Apache Kafka and uploading the tarball to S3 and commenting out the tarball verification code in the Dockerfile. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1877012641 I saw that but the action only allows me to pick a branch or tag. Ideally I'd like to run it on the pull request. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1876954982 @mimaison There is a GitHub Actions Workflow added for just that! You can check it out [here](https://github.com/apache/kafka/actions/workflows/docker_build_and_test.yml). Also there is helpful documentation for building, testing and releasing the docker image [here](https://github.com/apache/kafka/blob/trunk/docker/README.md) -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on PR #15048: URL: https://github.com/apache/kafka/pull/15048#issuecomment-1876867291 Is there a way to test this in the Apache CI or Github action? On my laptop `docker_build_test.py` fails due to some networking errors like ` Connection to node -1 (localhost/127.0.0.1:9092) could not be established.`. I've not had time to investigate it but it would be nice to perform checks in the CI. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1441555912
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
Review Comment:
Thanks for the suggestion. Incorporated
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r144171
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
Review Comment:
Fair point. Made the required changes
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
viktorsomogyi commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1440302423
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
Review Comment:
Parsing these 3 arguments would also be a good opportunity to validate and
convert these into `Path` objects. Using the API of `Path` also makes your code
more robust down the line when you append the config file names in
`prepareConfigs`.
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
+/*
+ * 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 kafka.docker
+
+import kafka.tools.StorageTool
+import kafka.utils.Exit
+
+import java.nio.charset.StandardCharsets
+import java.nio.file.{Files, Paths, StandardCopyOption, StandardOpenOption}
+
+object KafkaDockerWrapper {
+ def main(args: Array[String]): Unit = {
+if (args.length == 0) {
+ throw new RuntimeException(s"Error: No operation input provided. " +
+s"Please provide a valid operation: 'setup'.")
+}
+val operation = args.head
+val arguments = args.tail
+
+operation match {
+ case "setup" =>
+if (arguments.length != 3) {
+ val errMsg = "not enough arguments passed. Usage: " +
+"setup ,
"
+ System.err.println(errMsg)
+ Exit.exit(1, Some(errMsg))
+}
+val defaultConfigsDir = arguments(0)
+val mountedConfigsDir = arguments(1)
+val finalConfigsDir = arguments(2)
+try {
+ prepareConfigs(defaultConfigsDir, mountedConfigsDir, finalConfigsDir)
+} catch {
+ case e: Throwable =>
+val errMsg = s"error while preparing configs: ${e.getMessage}"
+System.err.println(errMsg)
+Exit.exit(1, Some(errMsg))
+}
+
+val formatCmd = formatStorageCmd(finalConfigsDir, envVars)
+StorageTool.main(formatCmd)
+ case _ =>
+throw new RuntimeException(s"Unknown operation $operation. " +
+ s"Please provide a valid operation: 'setup'.")
+}
+ }
+
+ import Constants._
+
+ private def formatStorageCmd(configsDir: String, env: Map[String, String]):
Array[String] = {
Review Comment:
While I agree that it works, it's not a nice solution. Would it be a big
refactor to use a specific method from `StorageTool` instead of `main`?
##
core/src/main/scala/kafka/docker/KafkaDockerWrapper.scala:
##
@@ -0,0 +1,218 @@
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1437520478
##
docker/jvm/launch:
##
@@ -28,25 +27,25 @@ fi
# the default is to pick the first IP (or network).
export KAFKA_JMX_HOSTNAME=${KAFKA_JMX_HOSTNAME:-$(hostname -i | cut -d" " -f1)}
-if [ "$KAFKA_JMX_PORT" ]; then
+if [ "${KAFKA_JMX_PORT-}" ]; then
# This ensures that the "if" section for JMX_PORT in kafka launch script
does not trigger.
export JMX_PORT=$KAFKA_JMX_PORT
-export KAFKA_JMX_OPTS="$KAFKA_JMX_OPTS
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
+export KAFKA_JMX_OPTS="${KAFKA_JMX_OPTS-}
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
fi
# Make a temp env variable to store user provided performance otps
-if [ -z "$KAFKA_JVM_PERFORMANCE_OPTS" ]; then
+if [ -z "${KAFKA_JVM_PERFORMANCE_OPTS-}" ]; then
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS=""
else
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS"
fi
# We will first use CDS for storage to format storage
-export KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
+export KAFKA_JVM_PERFORMANCE_OPTS="${KAFKA_JVM_PERFORMANCE_OPTS-}
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
echo "===> Using provided cluster id $CLUSTER_ID ..."
# A bit of a hack to not error out if the storage is already formatted. Need
storage-tool to support this
Review Comment:
Sounds good. Used `--ignored-formatted` flag of storage format command
inside the wrapper. This will allow us to restart the docker container without
running into an error, and thus remove the need for this hack
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
mimaison commented on code in PR #15048:
URL: https://github.com/apache/kafka/pull/15048#discussion_r1434227907
##
docker/jvm/launch:
##
@@ -28,25 +27,25 @@ fi
# the default is to pick the first IP (or network).
export KAFKA_JMX_HOSTNAME=${KAFKA_JMX_HOSTNAME:-$(hostname -i | cut -d" " -f1)}
-if [ "$KAFKA_JMX_PORT" ]; then
+if [ "${KAFKA_JMX_PORT-}" ]; then
# This ensures that the "if" section for JMX_PORT in kafka launch script
does not trigger.
export JMX_PORT=$KAFKA_JMX_PORT
-export KAFKA_JMX_OPTS="$KAFKA_JMX_OPTS
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
+export KAFKA_JMX_OPTS="${KAFKA_JMX_OPTS-}
-Djava.rmi.server.hostname=$KAFKA_JMX_HOSTNAME
-Dcom.sun.management.jmxremote.local.only=false
-Dcom.sun.management.jmxremote.rmi.port=$JMX_PORT
-Dcom.sun.management.jmxremote.port=$JMX_PORT"
fi
# Make a temp env variable to store user provided performance otps
-if [ -z "$KAFKA_JVM_PERFORMANCE_OPTS" ]; then
+if [ -z "${KAFKA_JVM_PERFORMANCE_OPTS-}" ]; then
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS=""
else
export TEMP_KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS"
fi
# We will first use CDS for storage to format storage
-export KAFKA_JVM_PERFORMANCE_OPTS="$KAFKA_JVM_PERFORMANCE_OPTS
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
+export KAFKA_JVM_PERFORMANCE_OPTS="${KAFKA_JVM_PERFORMANCE_OPTS-}
-XX:SharedArchiveFile=/opt/kafka/storage.jsa"
echo "===> Using provided cluster id $CLUSTER_ID ..."
# A bit of a hack to not error out if the storage is already formatted. Need
storage-tool to support this
Review Comment:
Could we handle that in the wrapper?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] KAFKA-16016: Add docker wrapper in core and remove docker utility script [kafka]
VedarthConfluent opened a new pull request, #15048: URL: https://github.com/apache/kafka/pull/15048 Migrates functionality provided by utility to Kafka core. This wrapper will be used to generate property files and format storage when invoked from docker container. ### 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
