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/10/13 15:29:20 UTC
[GitHub] [camel] gilvansfilho opened a new pull request, #8549: Cleanup jpa component
gilvansfilho opened a new pull request, #8549:
URL: https://github.com/apache/camel/pull/8549
Solving some pointings of sonar.
<!-- Uncomment and fill this section if your PR is not trivial
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean install -Psourcecheck` in your module with source check enabled to make sure basic checks pass and there are no checkstyle violations. A more thorough check will be performed on your pull request automatically.
Below are the contribution guidelines:
https://github.com/apache/camel/blob/main/CONTRIBUTING.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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1278500465
My only suggestion is regarding the commit message, that could be more specific with regards to the changes, so it makes it easier for us when bisecting or backporting.
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r999297104
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
I know how to use this but I really find it a bit confusing. But no problem, I will do it.
--
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] zhfeng commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997643562
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
I think we could ignore this issue as `volatile` here is used to avoiding creating multi `ExecutorService` in `receive(long timeout)`.
--
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] gilvansfilho commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1277801131
@orpiske as we talked about
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997177289
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
This will introduce another sonar pointing as [`volatile` should be used only with primitive fields](https://rules.sonarsource.com/java/RSPEC-3077).
So classes which implements ExecutorService interface should be care about `volatile` / concurrencie / multi thread access
--
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] orpiske commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997927981
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaEndpoint.java:
##########
@@ -185,6 +185,16 @@ protected String createEndpointUri() {
return "jpa" + (entityType != null ? "://" + entityType.getName() : "");
}
+ @Override
Review Comment:
I don't think this rule is relevant in this case. So, it should be safe to ignore Sonar (I should try to find a way to disable this 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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r999313183
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
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.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] orpiske merged pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske merged PR #8549:
URL: https://github.com/apache/camel/pull/8549
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997066841
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
Yes, there are. [If class is not `Serializable` then dont need to use transient](https://sonarcloud.io/project/issues?issues=AX4xQtpX2wn0z3gD78Js&open=AX4xQtpX2wn0z3gD78Js&id=apache_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] orpiske commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1277807246
Folks: I am helping @gilvansfilho understand about the contribution workflow for Camel and how to get started contributing with the project, so I'll take a look. Nonetheless, please do feel free to review and give insights about his contribution.
--
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] davsclaus commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
davsclaus commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997102157
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
Yes correct it should be volatile
--
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] zhfeng commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998277043
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
I agree and is there CAMEL issue for tracking these fixes?
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998398483
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaEndpoint.java:
##########
@@ -185,6 +185,16 @@ protected String createEndpointUri() {
return "jpa" + (entityType != null ? "://" + entityType.getName() : "");
}
+ @Override
Review Comment:
@orpiske `equals` and `hashCode` removed from last commit.
--
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] gilvansfilho commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1282981126
Tests failed here too because error when downloading some dependencies:
- https://repository.apache.org/snapshots/org/apache/camel/spi-annotations/3.20.0-SNAPSHOT/spi-annotations-3.20.0-20221018.103417-15.jar
- https://repository.apache.org/snapshots/org/apache/camel/camel-management-api/3.20.0-SNAPSHOT/camel-management-api-3.20.0-20221018.103417-15.jar
- https://repository.apache.org/snapshots/org/apache/camel/camel-support/3.20.0-SNAPSHOT/camel-support-3.20.0-20221018.103417-15.jar
All errors are like below:
```
[ERROR] Failed to execute goal on project camel-jpa: Could not resolve dependencies for project org.apache.camel:camel-jpa:jar:3.20.0-SNAPSHOT: The following artifacts could not be resolved: org.apache.camel:camel-support:jar:3.20.0-SNAPSHOT, org.apache.camel:camel-management-api:jar:3.20.0-SNAPSHOT, org.apache.camel:camel-util:jar:3.20.0-SNAPSHOT, org.apache.camel:camel-core-languages:jar:3.20.0-SNAPSHOT: Could not transfer artifact org.apache.camel:camel-support:jar:3.20.0-20221018.103417-15 from/to apache.snapshots (https://repository.apache.org/snapshots/): transfer failed for https://repository.apache.org/snapshots/org/apache/camel/camel-support/3.20.0-SNAPSHOT/camel-support-3.20.0-20221018.103417-15.jar: peer not authenticated -> [Help 1]
```
With wget I was able to download these dependencies and after put them on my local repo tests were successfully executed
--
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] davsclaus commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
davsclaus commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r996270805
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaEndpoint.java:
##########
@@ -185,6 +185,16 @@ protected String createEndpointUri() {
return "jpa" + (entityType != null ? "://" + entityType.getName() : "");
}
+ @Override
Review Comment:
I wonder why sonarcloud reports a problem that equals/hashCode needs to be overridden when they just call super?
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r996334893
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaEndpoint.java:
##########
@@ -185,6 +185,16 @@ protected String createEndpointUri() {
return "jpa" + (entityType != null ? "://" + entityType.getName() : "");
}
+ @Override
Review Comment:
Sonar cares about additional [subclass fields and the needeed of add them in equals and hash code methods](https://rules.sonarsource.com/java/RSPEC-2160). In that case this is not necessary as equality is based on context name and endpoint Uri so additional fields dont need to be considered. However sonar points as issue.
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997177289
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
This will introduce another sonar pointing as [`volatile` should be used only with primitive fields](https://rules.sonarsource.com/java/RSPEC-3077).
--
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] github-actions[bot] commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1282681604
### Components tested:
| Total | Tested | Failed :x: | Passed :white_check_mark: |
| --- | --- | --- | --- |
| 1 | 1 | 1 | 0 |
--
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] orpiske commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998282790
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
I am not aware of any ticket tracking this item in particular. However, it is [listed on our SonarCloud instance](https://sonarcloud.io/project/issues?resolved=false&rules=java%3AS3077&id=apache_camel).
Please, feel free to create a Jira ticket if you think we should track it.
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997105797
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
Should I change 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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997177289
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
This will introduce another sonar pointing as [`volatile` should be used only with primitive fields](https://rules.sonarsource.com/java/RSPEC-3077).
So classes which implements ExecutorService interface should be concerned about `volatile` / concurrency / multi thread access
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998689695
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
Code will be something like:
```
@Test
public void testJpaComponentConsumerHasLockModeType() throws Exception {
try (JpaComponent comp = new JpaComponent()) {
comp.setCamelContext(context);
assertNull(comp.getEntityManagerFactory());
assertNull(comp.getTransactionManager());
JpaEndpoint jpa
= (JpaEndpoint) comp.createEndpoint("jpa://" + SendEmail.class.getName() + "?lockModeType=PESSIMISTIC_WRITE");
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
} catch (Exception e) {
throw e; //or e.printStackTrace();
}
}
```
Once we will not handle the exception I believe is not necessary use try-with-resource.
--
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] orpiske commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1283978666
> ### Components tested:
> Total Tested Failed ❌ Passed ✅
> 7 7 1 6
it seems to me that all comments were addressed.
Also, the failure is unrelated to this PR, so I think we are good to go.
Merging it ...
--
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] essobedo commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
essobedo commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998720893
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
Why do you catch the exception ? You could simply throw it
--
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] github-actions[bot] commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1283956365
### Components tested:
| Total | Tested | Failed :x: | Passed :white_check_mark: |
| --- | --- | --- | --- |
| 7 | 7 | 1 | 6 |
--
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] github-actions[bot] commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1277801042
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:warning: Please note that the changes on this PR may be **tested automatically**.
If necessary Apache Camel Committers may access logs and test results in the job summaries!
--
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] essobedo commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
essobedo commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998407324
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
What about using a try-with-resource statement instead?
--
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] orpiske commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
orpiske commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997952712
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
> I think we could ignore this issue as `volatile` here is used to avoiding creating multi `ExecutorService` in `receive(long timeout)`.
I don't think volatile is enough to protect the code in this particular scenario. A synchronized block / lock would be the correct to ensure this. AFAIK, it only guarantees the visibility updates of itself (and other variables declared around it) but cannot protect against concurrent writes of the variable. And, since we have 2 operations happening on that variable in that particular block (a fetch - from memory - and a write - to memory) it would still be entirely possible for 2 threads to concurrently fetch a null value at the same time.
```
// need to use a thread pool to perform the task so we can support timeout
if (executorService == null) {
executorService = getEndpoint().getComponent().getOrCreatePollingConsumerExecutorService();
}
```
That said ... I have seen this pattern used elsewhere in the code and I always wanted to fix it, but given a lot of other things we should take into consideration before fixing it, I've always left it aside as suggested fix for a major version.
So, TL,DR: IMHO, `volatile` is not OK, but we can leave it be for a larger fix in the future.
--
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] essobedo commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
essobedo commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998921526
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
No because the try-with-resources statement has been created for this purpose. Read this https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html for more details.
--
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] github-actions[bot] commented on pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #8549:
URL: https://github.com/apache/camel/pull/8549#issuecomment-1277888651
### Components tested:
| Total | Tested | Failed :x: | Passed :white_check_mark: |
| --- | --- | --- | --- |
| 1 | 1 | 0 | 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] zhfeng commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997050476
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
why remove `transient` here? Is there an issue sonar reported?
--
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] zhfeng commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
zhfeng commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997092950
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
Hmm, it is interesting! I wonder if the modifier should be `volatile` since there might be multi polling consumers? @orpiske wdyt?
--
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] davsclaus commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
davsclaus commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r997111340
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
Yes pleaase
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998744431
##########
components/camel-jpa/src/test/java/org/apache/camel/component/jpa/JpaComponentTest.java:
##########
@@ -44,6 +44,7 @@ public void testJpaComponentConsumerHasLockModeType() throws Exception {
JpaConsumer consumer = (JpaConsumer) jpa.createConsumer(null);
assertEquals(LockModeType.PESSIMISTIC_WRITE, consumer.getLockModeType());
+ comp.close();
Review Comment:
That is true. But don't is a bit confusing use try without catch?
--
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] gilvansfilho commented on a diff in pull request #8549: Cleanup jpa component
Posted by GitBox <gi...@apache.org>.
gilvansfilho commented on code in PR #8549:
URL: https://github.com/apache/camel/pull/8549#discussion_r998399266
##########
components/camel-jpa/src/main/java/org/apache/camel/component/jpa/JpaPollingConsumer.java:
##########
@@ -44,7 +44,7 @@ public class JpaPollingConsumer extends PollingConsumerSupport {
private static final Logger LOG = LoggerFactory.getLogger(JpaPollingConsumer.class);
- private transient ExecutorService executorService;
+ private ExecutorService executorService;
Review Comment:
@orpiske `volatile` added as with talked about
--
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