You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2020/10/14 14:35:04 UTC
[GitHub] [camel-quarkus] aldettinger opened a new pull request #1920: Added nsq native support fixes #1722
aldettinger opened a new pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920
[ ] An issue should be filed for the change unless this is a trivial change (fixing a typo or similar). One issue should ideally be fixed by not more than one commit and the other way round, each commit should fix just one issue, without pulling in other changes.
[ ] Each commit in the pull request should have a meaningful and properly spelled subject line and body. Copying the title of the associated issue is typically enough. Please include the issue number in the commit message prefixed by #.
[ ] The pull request description should explain what the pull request does, how, and why. If the info is available in the associated issue or some other external document, a link is enough.
[ ] Phrases like Fix #<issueNumber> or Fixes #<issueNumber> will auto-close the named issue upon merging the pull request. Using them is typically a good idea.
[ ] Please run mvn process-resources -Pformat (and amend the changes if necessary) before sending the pull request.
[ ] Contributor guide is your good friend: https://camel.apache.org/camel-quarkus/latest/contributor-guide.html
----------------------------------------------------------------
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] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504775637
##########
File path: extensions/nsq/deployment/src/main/java/org/apache/camel/quarkus/component/nsq/deployment/NsqProcessor.java
##########
@@ -16,12 +16,10 @@
*/
package org.apache.camel.quarkus.component.nsq.deployment;
+import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
-import io.quarkus.deployment.annotations.ExecutionTime;
-import io.quarkus.deployment.annotations.Record;
import io.quarkus.deployment.builditem.FeatureBuildItem;
-import io.quarkus.deployment.pkg.steps.NativeBuild;
-import org.apache.camel.quarkus.core.JvmOnlyRecorder;
+import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import org.jboss.logging.Logger;
class NsqProcessor {
Review comment:
Sure, it's always a pleasure to remove useless code :)
----------------------------------------------------------------
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] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504774916
##########
File path: integration-tests/nsq/src/main/java/org/apache/camel/quarkus/component/nsq/it/NsqLogger.java
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.camel.quarkus.component.nsq.it;
+
+import org.jboss.logging.Logger;
+
+/**
+ * Quarkus is not able to display logs in JVM mode when running NSQ surefire tests.
+ * This class intent to workaround this issue.
+ */
+public class NsqLogger {
+
+ public static void log(Logger logger, String format, Object... args) {
+ String log = String.format(format, args);
+ System.out.println(log);
Review comment:
The sysout call is there to have some logs during tests in JVM mode, otherwise it's really hard to catch what's happening. My understanding from discussion with quarkus guys is that it will still be needed, but I plan to check in the follow up.
----------------------------------------------------------------
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] [camel-quarkus] jamesnetherton commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
jamesnetherton commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504742874
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -23,17 +23,22 @@
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.camel.quarkus</groupId>
- <artifactId>camel-quarkus-build-parent-it</artifactId>
+ <artifactId>camel-quarkus-integration-tests</artifactId>
<version>1.2.0-SNAPSHOT</version>
- <relativePath>../../../poms/build-parent-it/pom.xml</relativePath>
</parent>
- <artifactId>camel-quarkus-nsq-integration-test</artifactId>
- <name>Camel Quarkus :: NSQ :: Integration Test</name>
+ <artifactId>camel-quarkus-integration-test-nsq</artifactId>
+ <name>Camel Quarkus :: Integration Tests :: NSQ</name>
<description>Integration tests for Camel Quarkus NSQ extension</description>
<dependencyManagement>
<dependencies>
+ <!-- Workaround https://github.com/quarkusio/quarkus/issues/12659 waiting for Quarkus > 1.9.1.CR1 -->
Review comment:
1.0.5 is included in Quarkus 1.9.0. Once the upgrade PR is merged, this won't be necessary.
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -49,6 +54,10 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-nsq</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-direct</artifactId>
Review comment:
Please run `mvn process-resources -Pformat` to regenerate the deployment dependencies
##########
File path: integration-tests/nsq/src/main/java/org/apache/camel/quarkus/component/nsq/it/NsqLogger.java
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.camel.quarkus.component.nsq.it;
+
+import org.jboss.logging.Logger;
+
+/**
+ * Quarkus is not able to display logs in JVM mode when running NSQ surefire tests.
+ * This class intent to workaround this issue.
+ */
+public class NsqLogger {
+
+ public static void log(Logger logger, String format, Object... args) {
+ String log = String.format(format, args);
+ System.out.println(log);
Review comment:
Is this class still needed after we get `jboss-logmanager-embedded` 1.0.5? And do we need the sysout calls?
##########
File path: extensions/nsq/deployment/src/main/java/org/apache/camel/quarkus/component/nsq/deployment/NsqProcessor.java
##########
@@ -16,12 +16,10 @@
*/
package org.apache.camel.quarkus.component.nsq.deployment;
+import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
-import io.quarkus.deployment.annotations.ExecutionTime;
-import io.quarkus.deployment.annotations.Record;
import io.quarkus.deployment.builditem.FeatureBuildItem;
-import io.quarkus.deployment.pkg.steps.NativeBuild;
-import org.apache.camel.quarkus.core.JvmOnlyRecorder;
+import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import org.jboss.logging.Logger;
class NsqProcessor {
Review comment:
Small nit - can you remove the now unused LOG field.
----------------------------------------------------------------
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] [camel-quarkus] jamesnetherton commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
jamesnetherton commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504773599
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -23,17 +23,22 @@
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.camel.quarkus</groupId>
- <artifactId>camel-quarkus-build-parent-it</artifactId>
+ <artifactId>camel-quarkus-integration-tests</artifactId>
<version>1.2.0-SNAPSHOT</version>
- <relativePath>../../../poms/build-parent-it/pom.xml</relativePath>
</parent>
- <artifactId>camel-quarkus-nsq-integration-test</artifactId>
- <name>Camel Quarkus :: NSQ :: Integration Test</name>
+ <artifactId>camel-quarkus-integration-test-nsq</artifactId>
+ <name>Camel Quarkus :: Integration Tests :: NSQ</name>
<description>Integration tests for Camel Quarkus NSQ extension</description>
<dependencyManagement>
<dependencies>
+ <!-- Workaround https://github.com/quarkusio/quarkus/issues/12659 waiting for Quarkus > 1.9.1.CR1 -->
Review comment:
Maybe wait for https://github.com/apache/camel-quarkus/pull/1921 and rebase on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504831118
##########
File path: integration-tests/nsq/src/main/java/org/apache/camel/quarkus/component/nsq/it/NsqLogger.java
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.camel.quarkus.component.nsq.it;
+
+import org.jboss.logging.Logger;
+
+/**
+ * Quarkus is not able to display logs in JVM mode when running NSQ surefire tests.
+ * This class intent to workaround this issue.
+ */
+public class NsqLogger {
+
+ public static void log(Logger logger, String format, Object... args) {
+ String log = String.format(format, args);
+ System.out.println(log);
Review comment:
I've tested and it is still 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] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504778932
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -23,17 +23,22 @@
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.camel.quarkus</groupId>
- <artifactId>camel-quarkus-build-parent-it</artifactId>
+ <artifactId>camel-quarkus-integration-tests</artifactId>
<version>1.2.0-SNAPSHOT</version>
- <relativePath>../../../poms/build-parent-it/pom.xml</relativePath>
</parent>
- <artifactId>camel-quarkus-nsq-integration-test</artifactId>
- <name>Camel Quarkus :: NSQ :: Integration Test</name>
+ <artifactId>camel-quarkus-integration-test-nsq</artifactId>
+ <name>Camel Quarkus :: Integration Tests :: NSQ</name>
<description>Integration tests for Camel Quarkus NSQ extension</description>
<dependencyManagement>
<dependencies>
+ <!-- Workaround https://github.com/quarkusio/quarkus/issues/12659 waiting for Quarkus > 1.9.1.CR1 -->
Review comment:
yes good idea
----------------------------------------------------------------
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] [camel-quarkus] aldettinger merged pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger merged pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920
----------------------------------------------------------------
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] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504769056
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -23,17 +23,22 @@
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.camel.quarkus</groupId>
- <artifactId>camel-quarkus-build-parent-it</artifactId>
+ <artifactId>camel-quarkus-integration-tests</artifactId>
<version>1.2.0-SNAPSHOT</version>
- <relativePath>../../../poms/build-parent-it/pom.xml</relativePath>
</parent>
- <artifactId>camel-quarkus-nsq-integration-test</artifactId>
- <name>Camel Quarkus :: NSQ :: Integration Test</name>
+ <artifactId>camel-quarkus-integration-test-nsq</artifactId>
+ <name>Camel Quarkus :: Integration Tests :: NSQ</name>
<description>Integration tests for Camel Quarkus NSQ extension</description>
<dependencyManagement>
<dependencies>
+ <!-- Workaround https://github.com/quarkusio/quarkus/issues/12659 waiting for Quarkus > 1.9.1.CR1 -->
Review comment:
Yes, I plan to open a fup not to forget. And by the way, this is quarkus > 1.9.0.CR1. I'm gonna fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504773006
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -49,6 +54,10 @@
<groupId>org.apache.camel.quarkus</groupId>
<artifactId>camel-quarkus-nsq</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.camel.quarkus</groupId>
+ <artifactId>camel-quarkus-direct</artifactId>
Review comment:
Ok, I see the update when running `mvn process-resources -Pformat -N` from the source tree root 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #1920: Added nsq native support fixes #1722
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #1920:
URL: https://github.com/apache/camel-quarkus/pull/1920#discussion_r504830899
##########
File path: integration-tests/nsq/pom.xml
##########
@@ -23,17 +23,22 @@
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.camel.quarkus</groupId>
- <artifactId>camel-quarkus-build-parent-it</artifactId>
+ <artifactId>camel-quarkus-integration-tests</artifactId>
<version>1.2.0-SNAPSHOT</version>
- <relativePath>../../../poms/build-parent-it/pom.xml</relativePath>
</parent>
- <artifactId>camel-quarkus-nsq-integration-test</artifactId>
- <name>Camel Quarkus :: NSQ :: Integration Test</name>
+ <artifactId>camel-quarkus-integration-test-nsq</artifactId>
+ <name>Camel Quarkus :: Integration Tests :: NSQ</name>
<description>Integration tests for Camel Quarkus NSQ extension</description>
<dependencyManagement>
<dependencies>
+ <!-- Workaround https://github.com/quarkusio/quarkus/issues/12659 waiting for Quarkus > 1.9.1.CR1 -->
Review comment:
done
----------------------------------------------------------------
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