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 2022/03/29 15:05:16 UTC
[GitHub] [camel-quarkus] aldettinger opened a new pull request #3681: paho: Add test coverage for file persistence #3680
aldettinger opened a new pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681
<!-- Uncomment and fill this section if your PR is not trivial
[ ] 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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839675593
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
@aldettinger The root cause is we are missing ```RandomAccessFile``` reflection config which fails to get the file lock.
So adding
```
p.produce(new ReflectiveClassBuildItem(true, false, RandomAccessFile.class));
```
resloves the problem.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#issuecomment-1083176213
@aldettinger Do you think this test could be also for ```paho-mqtt5``` ?
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839610460
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
A theroy could be that the lock is not being released (in native mode only then):
https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/FileLock.java#L76
Maybe, I messed up the reflection config and `fileLock.getClass().getMethod("release",new Class[]{});` is throwing ?
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
A theory could be that the lock is not being released (in native mode only then):
https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/FileLock.java#L76
Maybe, I messed up the reflection config and `fileLock.getClass().getMethod("release",new Class[]{});` is throwing ?
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839562609
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
yeah, the ```clientDir``` is empty. I just think if there is the other thread opens the directory which could cause the deletion failing. unfortunately, ```java.io.File.delete()``` only return ```false``` but we don't know the reason. Perhaps ```java.nio.file.Files.delete(Path)``` can give us more details. I will keep investigating.
Both ```camel-paho``` and ```camel-paho-mqtt5' lack this file persistence test, so I think it could be better to open a issue in camel.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839572682
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
Ok, that would be a threading issue that we just see more often in native mode then. Really nice @zhfeng :+1:
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839530946
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
Hey, great catch. Would you by chance be able to check whether the `clientDir` is empty before deletion in native and JVM ? It might be a reason to return false.
Tracking this issue is definitely a good idea. Should the issue be opened in camel-quarkus or camel, I don't know. What do you think ?
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839680192
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
Oh, great catch. I take a note to test this with mqtt3 too :)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839589662
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
Ha, intersting ! ```java.nio.file.Files.delete(Path)``` throws ```java.nio.file.DirectoryNotEmptyException```. There is ```.lck``` in the directory. It looks like the client still holds a ```FileLock``` when clear() method is invoked.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839414873
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
yeah, the similar situation with mqtt5 but I have to add ```cleanStart=false```. I took some debug works by adding some ```system.out.println``` and [clientDir.delete()](https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/persist/MqttDefaultFilePersistence.java#L298) return ```false``` in jvm mode but ```true``` in native mode.
It might be worth to raise a upstream CAMEL issue track this problem.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger merged pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger merged pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#issuecomment-1083268725
@zhfeng I've added this test because paho mqtt3 has a class using reflection https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/internal/FileLock.java#L33. This class is used when camel is configured with FilePersistence.
So, If there are mqtt5 file classes using reflection in somes camel scenarios, I would say it's worth taking 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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] aldettinger commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
aldettinger commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839309469
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
Indeed, I have the same with mqtt3. In native with cleanSession = true, there is a NullPointerException. So far I've been able to investigate at this stage, it appears that the clientDir is deleted.
The deletion show up only in native mode, but I suspect it is a more general race condition. Perharps even in camel-paho itself. At this stage, I was not able to fully qualify this bug. cleanSession = false allow to test with file persistence without this bug.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#issuecomment-1083280166
Thanks @aldettinger yeah, v5 uses the same FileLock impl https://github.com/eclipse/paho.mqtt.java/blob/master/org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/internal/FileLock.java. I will add one.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-quarkus] zhfeng commented on a change in pull request #3681: paho: Add test coverage for file persistence #3680
Posted by GitBox <gi...@apache.org>.
zhfeng commented on a change in pull request #3681:
URL: https://github.com/apache/camel-quarkus/pull/3681#discussion_r839109020
##########
File path: integration-tests/paho/src/main/java/org/apache/camel/quarkus/component/paho/PahoResource.java
##########
@@ -87,4 +88,11 @@ public String mqttExceptionDuringReconnectShouldSucceed() {
return mqex.getMessage();
}
+ @Path("/readThenWriteWithFilePersistenceShouldSucceed")
+ @GET
+ @Produces(MediaType.TEXT_PLAIN)
+ public String readThenWriteWithFilePersistenceShouldSucceed(@QueryParam("message") String message) throws Exception {
+ producerTemplate.requestBody("paho:withFilePersistence?retained=true&cleanSession=false&persistence=FILE", message);
Review comment:
if I remove ```cleanSession=false``` , the native test starts to fail but the jvm test is OK. I just wonder if it is a workaround for the native test or it is a limitination?
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org