You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "mannoopj (via GitHub)" <gi...@apache.org> on 2024/11/04 14:59:33 UTC
[PR] Add git support for compatibility checker [kafka]
mannoopj opened a new pull request, #17684:
URL: https://github.com/apache/kafka/pull/17684
(no comment)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829719740
##########
generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java:
##########
@@ -91,4 +102,54 @@ static MessageSpec readMessageSpecFromFile(String schemaPath) {
throw new RuntimeException("Unable to parse file as MessageSpec: " + schemaPath, e);
}
}
+
+ /**
+ * Return a MessageSpec file give file contents.
+ *
+ * @param contents The path to read the file from.
+ * @return The MessageSpec.
+ */
+ static MessageSpec readMessageSpecFromString(String contents) {
+ try {
+ File tempFile = new File("temp.txt");
+ tempFile.createNewFile();
+ Files.write(Paths.get("temp.txt"), contents.getBytes());
+ MessageSpec messageSpec = MessageGenerator.JSON_SERDE.readValue(tempFile, MessageSpec.class);
+ tempFile.deleteOnExit();
+ return messageSpec;
+ } catch (Exception e) {
+ throw new RuntimeException("Unable to parse file as MessageSpec: " + contents, e);
+ }
+ }
+
+
+ /**
+ * Read a MessageSpec file from remote git repo.
+ *
+ * @param fileCheckMetadata The file to read from remote git repo.
+ * @return The file contents.
+ */
+ static String GetDataFromGit(String fileCheckMetadata) throws IOException {
+ String gitPath = String.valueOf(Paths.get("").toAbsolutePath()).replaceFirst("/generator", "");
+ Git git = Git.open(new File(gitPath + "/.git"));
+ Repository repository = git.getRepository();
+ Ref head = git.getRepository().getRefDatabase().firstExactRef("refs/heads/trunk");
+
+ try (RevWalk revWalk = new RevWalk(repository)) {
+ RevCommit commit = revWalk.parseCommit(head.getObjectId());
+ RevTree tree = commit.getTree();
+ try (TreeWalk treeWalk = new TreeWalk(repository)) {
+ treeWalk.addTree(tree);
+ treeWalk.setRecursive(true);
+ treeWalk.setFilter(PathFilter.create(String.valueOf(Paths.get("metadata/src/main/resources/common/metadata/" + fileCheckMetadata))));
Review Comment:
please use a constant for `metadata/src/main/resources/common/metadata`
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1852320989
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
I changed to code so it works anywhere within the kafka directory.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1852320989
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
I changed to code so it works anywhere within the kafka directory. Im trying to get the absolute path to the kafka directory
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1855219609
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,6 +90,23 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String filePath = "/metadata/src/main/resources/common/metadata/" + namespace.getString("file");
+ Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
+ while (!rootKafkaDirectory.endsWith("kafka")) {
Review Comment:
this condition depends on the project folder name. What we're actually looking for is the Git folder, so maybe we could change the condition to `while (!new File(rootKafkaDirectory.toFile(), ".git").exists())`, which would allow us to use a different folder name.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829718558
##########
generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java:
##########
@@ -91,4 +102,54 @@ static MessageSpec readMessageSpecFromFile(String schemaPath) {
throw new RuntimeException("Unable to parse file as MessageSpec: " + schemaPath, e);
}
}
+
+ /**
+ * Return a MessageSpec file give file contents.
+ *
+ * @param contents The path to read the file from.
+ * @return The MessageSpec.
+ */
+ static MessageSpec readMessageSpecFromString(String contents) {
+ try {
+ File tempFile = new File("temp.txt");
+ tempFile.createNewFile();
+ Files.write(Paths.get("temp.txt"), contents.getBytes());
+ MessageSpec messageSpec = MessageGenerator.JSON_SERDE.readValue(tempFile, MessageSpec.class);
+ tempFile.deleteOnExit();
+ return messageSpec;
+ } catch (Exception e) {
+ throw new RuntimeException("Unable to parse file as MessageSpec: " + contents, e);
+ }
+ }
+
+
+ /**
+ * Read a MessageSpec file from remote git repo.
+ *
+ * @param fileCheckMetadata The file to read from remote git repo.
+ * @return The file contents.
+ */
+ static String GetDataFromGit(String fileCheckMetadata) throws IOException {
Review Comment:
I don't understand why the parameter is named `fileCheckMetadata`, can you explain?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1842897550
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
If we know where the git repo root is, why can't we just change to that directory rather than requiring a specific current working directory?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829720328
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -56,6 +73,11 @@ public static void run(
evolutionVerifierParser.addArgument("--path2", "-2").
required(true).
help("The final schema JSON path.");
+ Subparser evolutionGitVerifierParser = subparsers.addParser("verify-evolution-git").
+ help(" Verify that an evolution of a JSon file is valid using git. ");
Review Comment:
`JSon` ?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1838414516
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
Ok yeah I added documentation in the help output stating that this tool is expected to be run from the `generator` directory. That was the assumption I was working under. Getting the git root repo is done in the `GetDataFromGit` function in `CheckerUtil.java`
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1864271992
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,6 +90,23 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String filePath = "/metadata/src/main/resources/common/metadata/" + namespace.getString("file");
+ Path rootKafkaDirectory = Paths.get("").toAbsolutePath();
+ while (!rootKafkaDirectory.endsWith("kafka")) {
Review Comment:
I have filed https://issues.apache.org/jira/browse/KAFKA-18128 to fix it and another failed test related to this PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1842875306
##########
generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java:
##########
@@ -91,4 +102,54 @@ static MessageSpec readMessageSpecFromFile(String schemaPath) {
throw new RuntimeException("Unable to parse file as MessageSpec: " + schemaPath, e);
}
}
+
+ /**
+ * Return a MessageSpec file give file contents.
+ *
+ * @param contents The path to read the file from.
+ * @return The MessageSpec.
+ */
+ static MessageSpec readMessageSpecFromString(String contents) {
+ try {
+ File tempFile = new File("temp.txt");
+ tempFile.createNewFile();
+ Files.write(Paths.get("temp.txt"), contents.getBytes());
+ MessageSpec messageSpec = MessageGenerator.JSON_SERDE.readValue(tempFile, MessageSpec.class);
+ tempFile.deleteOnExit();
+ return messageSpec;
+ } catch (Exception e) {
+ throw new RuntimeException("Unable to parse file as MessageSpec: " + contents, e);
+ }
+ }
+
+
+ /**
+ * Read a MessageSpec file from remote git repo.
+ *
+ * @param fileCheckMetadata The file to read from remote git repo.
+ * @return The file contents.
+ */
+ static String GetDataFromGit(String fileCheckMetadata) throws IOException {
Review Comment:
Was to check the metadata schema file, the variable is renamed 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1852320989
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
I changed to code so it works anywhere within the kafka directory. Im trying to get the absolute path to the kafka directory so that I can use that to git repo through jgit
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829724023
##########
generator/src/main/java/org/apache/kafka/message/checker/MetadataSchemaCheckerTool.java:
##########
@@ -81,8 +103,18 @@ public static void run(
", and path2: " + path2);
break;
}
+ case "verify-evolution-git": {
+ String fileCheckMetadata = namespace.getString("file");
+ String gitContent = GetDataFromGit(fileCheckMetadata);
+ EvolutionVerifier verifier = new EvolutionVerifier(
+ CheckerUtils.readMessageSpecFromFile(Paths.get("").toAbsolutePath().getParent() + "/metadata/src/main/resources/common/metadata/" + fileCheckMetadata),
Review Comment:
in general I don't understand what the strategy is here for paths. if you are using a relative path, you need to document what directory we need to be in. how are we locating the git repository root?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829724436
##########
tools/src/main/java/org/apache/kafka/tools/ClusterTool.java:
##########
@@ -37,6 +37,8 @@
import static net.sourceforge.argparse4j.impl.Arguments.store;
+import org.eclipse.jgit.api.Git;
Review Comment:
why is this import needed here?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe merged PR #17684:
URL: https://github.com/apache/kafka/pull/17684
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1852195231
##########
checkstyle/suppressions.xml:
##########
@@ -32,6 +32,8 @@
files="(ApiMessageType|FieldSpec|MessageDataGenerator|KafkaConsumerTest).java"/>
<suppress checks="MethodLength"
files="(FieldSpec|MessageDataGenerator).java"/>
+ <suppress checks="ImportControl"
Review Comment:
Was having trouble testing to so added it in the suppressions. Figured out I was having a local issue so this is just added to the allowed import controls 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1842874491
##########
tools/src/main/java/org/apache/kafka/tools/ClusterTool.java:
##########
@@ -37,6 +37,8 @@
import static net.sourceforge.argparse4j.impl.Arguments.store;
+import org.eclipse.jgit.api.Git;
Review Comment:
not 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1829717644
##########
generator/src/main/java/org/apache/kafka/message/checker/CheckerUtils.java:
##########
@@ -91,4 +102,54 @@ static MessageSpec readMessageSpecFromFile(String schemaPath) {
throw new RuntimeException("Unable to parse file as MessageSpec: " + schemaPath, e);
}
}
+
+ /**
+ * Return a MessageSpec file give file contents.
+ *
+ * @param contents The path to read the file from.
+ * @return The MessageSpec.
+ */
+ static MessageSpec readMessageSpecFromString(String contents) {
Review Comment:
we don't need to create a file to translate a string to a MessageSpec, we can do this directly.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1850761688
##########
checkstyle/suppressions.xml:
##########
@@ -32,6 +32,8 @@
files="(ApiMessageType|FieldSpec|MessageDataGenerator|KafkaConsumerTest).java"/>
<suppress checks="MethodLength"
files="(FieldSpec|MessageDataGenerator).java"/>
+ <suppress checks="ImportControl"
Review Comment:
why can we not just list `org.eclipse.jgit` as allowed?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "mannoopj (via GitHub)" <gi...@apache.org>.
mannoopj commented on code in PR #17684:
URL: https://github.com/apache/kafka/pull/17684#discussion_r1852195231
##########
checkstyle/suppressions.xml:
##########
@@ -32,6 +32,8 @@
files="(ApiMessageType|FieldSpec|MessageDataGenerator|KafkaConsumerTest).java"/>
<suppress checks="MethodLength"
files="(FieldSpec|MessageDataGenerator).java"/>
+ <suppress checks="ImportControl"
Review Comment:
Was having trouble testing to sp added it in the suppressions. Figured out I was having a local issue so this is just added to allowed 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add git support for compatibility checker [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #17684:
URL: https://github.com/apache/kafka/pull/17684#issuecomment-2492519514
@mannoopj : there are checkstyle issues, can you fix?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org