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 2021/04/14 16:57:46 UTC
[GitHub] [camel] samratdhillon opened a new pull request #5370: Feature/fixing zipkin spans
samratdhillon opened a new pull request #5370:
URL: https://github.com/apache/camel/pull/5370
Fixing CAMEL-16509
When copying Exchange properties, provide a mechanism to set the copy object as property value instead of the original value object. This will allow the properties to be safely shared between Exchange objects operating in parallelProcessing and in this case ZipkinState will be easily shared without it getting corrupted.
--
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] samratdhillon commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
samratdhillon commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613530035
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
@davsclaus Thanks for the advice. I think setting the flag via ExtendedExchange would be the best option.
--
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] davsclaus closed pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
davsclaus closed pull request #5370:
URL: https://github.com/apache/camel/pull/5370
--
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] davsclaus commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613462610
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
Yes we do not want core comprimised with this due to zipkin storing state like that. The core should be lean and fast.
--
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] samratdhillon commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
samratdhillon commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613694900
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
I have incorporated your feedback and also an alternate approach and IMHO cleaner one has been implemented in another PR https://github.com/apache/camel/pull/5373
--
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] davsclaus commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613446544
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
Sorry we do not want this kind in the core. Can you maybe try to come up with a different solution.
--
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] davsclaus commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r614030231
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
Oh that is great work that you implemented two different solutions. I agree with you that the alternative is the better choice, so lets close 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel] samratdhillon commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
samratdhillon commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613453034
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
@davsclaus Are you suggesting to totally avoid changes in core, or avoid these specific changes in core? I initially did come up with another approach for camel-zipkin but it is far more complicated and honestly does not address the real problem.
The span problem I am pretty sure exists in camel opentracing and opentelemetry as well because both of them use camel-tracing and if you look at [ActiveSpanManager](https://github.com/apache/camel/blob/master/components/camel-tracing/src/main/java/org/apache/camel/tracing/ActiveSpanManager.java) it also uses Exchange property mechanism to set ACTIVE_SPAN_PROPERTY which would get corrupted when using parallelProcessing with multicast.
My suggested approach would easily fix issues in opentracing and opentelemetry as well.
--
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] davsclaus commented on a change in pull request #5370: Feature/fixing zipkin spans
Posted by GitBox <gi...@apache.org>.
davsclaus commented on a change in pull request #5370:
URL: https://github.com/apache/camel/pull/5370#discussion_r613465975
##########
File path: core/camel-support/src/main/java/org/apache/camel/support/AbstractExchange.java
##########
@@ -189,7 +189,15 @@ public Exchange copy() {
@SuppressWarnings("unchecked")
private void safeCopyProperties(Map<String, Object> source, Map<String, Object> target) {
- target.putAll(source);
+ source.entrySet().stream().forEach(entry -> {
Review comment:
Instead of doing this expensive check always, then find out some way for zipkin et all to turn on a flag on the exchange (via ExtendedExchange) to tell it that it should do deep clone safe copy mode, then this is only in use when you use these special zipkin components.
Or instead of storing the zipkin state as exchange property, store it on some zipkin bean, that handles this and stores the data per exchange id or something.
--
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