-
Notifications
You must be signed in to change notification settings - Fork 642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ISSUE #3402]"getExtension()" can return null.[Trace] #3632
Conversation
Included "Object.requireNonNull()"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/trace/Trace.java
Outdated
Show resolved
Hide resolved
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/trace/Trace.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @harshithasudhakar CI not pass, Please check it
I have rectified the issue now. |
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/trace/Trace.java
Outdated
Show resolved
Hide resolved
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/trace/Trace.java
Outdated
Show resolved
Hide resolved
eventmesh-runtime/src/main/java/org/apache/eventmesh/runtime/trace/Trace.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The judgment of ternary operation ensures that cloudEvent. getExtension (entry)
cannot return null. Objects.requireNonNull()
is redundant. @harshithasudhakar @xwm1992
@@ -140,7 +141,7 @@ public Span addTraceInfoToSpan(Span span, CloudEvent cloudEvent) { | |||
} | |||
|
|||
for (String entry : cloudEvent.getExtensionNames()) { | |||
span.setAttribute(entry, cloudEvent.getExtension(entry) == null ? "" : cloudEvent.getExtension(entry).toString()); | |||
span.setAttribute(entry, cloudEvent.getExtension(entry) != null ? cloudEvent.getExtension(entry).toString() : ""); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you see there is no problem with the original code here. The other one is in the same situation.
In conclusion, this issue should not be posted. So let's wait the maintainer to make the final decision.
@pandaapo 你好👋 我想找一些初级的issue但是看这个貌似已经解决了但是没有关闭,这里还有其他直接commit的点吗? hello 👋 I am looking for some beginner issues, but it seems that they have been resolved but not closed. Are there any other direct commit points here? |
@scwlkq Hi you can try this issue #4502 ,If you are interested, I can assign this issue to you. |
Codecov Report
@@ Coverage Diff @@
## master #3632 +/- ##
============================================
- Coverage 15.47% 15.47% -0.01%
Complexity 1452 1452
============================================
Files 691 691
Lines 28106 28112 +6
Branches 2626 2629 +3
============================================
Hits 4349 4349
- Misses 23312 23318 +6
Partials 445 445
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What is the significance of these modifications in this PR? @mxsm |
Any progress can be made in this PR? |
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3632 +/- ##
============================================
- Coverage 15.47% 15.47% -0.01%
Complexity 1452 1452
============================================
Files 691 691
Lines 28106 28112 +6
Branches 2626 2629 +3
============================================
Hits 4349 4349
- Misses 23312 23318 +6
Partials 445 445 ☔ View full report in Codecov by Sentry. |
Span span = context != null ? context.get(SpanKey.SERVER_KEY) : null; | ||
return addTraceInfoToSpan(span, cloudEvent); | ||
|
||
if (span == null) { | ||
log.warn("span is null when finishSpan"); | ||
return null; | ||
} | ||
|
||
//add trace info | ||
for (String entry : cloudEvent.getExtensionNames()) { | ||
span.setAttribute(entry, cloudEvent.getExtension(entry) == null ? "" : Objects.requireNonNull(cloudEvent.getExtension(entry)).toString()); | ||
} | ||
return span; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to inline the addTraceInfoToSpan
method here? I think it is enough to add a requireNonNull
method in the "add trace info" step of addTraceInfoToSpan
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is enough to add a
requireNonNull
method in the "add trace info" step ofaddTraceInfoToSpan
method.
I found that your current suggestion contradicts your previous one (#3632 (comment)). So, why you now think it's needed to add an extra requireNonNull
to addTraceInfoToSpan
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pandaapo Excuse me for overlooking some folded context earlier. I initially intended to point out that the addTraceInfoToSpan
method should not be inlined here. However, it appears that further review is unnecessary as the issue corresponding to this PR is invalid. I will close the issue and label this PR as invalid
.
@harshithasudhakar Thank you very much for your contribution. Please feel free to select other valuable issues to work on.
It has been 60 days since the last activity on this pull request. I am reaching out here to gently remind you that the Apache EventMesh community values every pull request, and please feel free to get in touch with the reviewers at any time. They are available to assist you in advancing the progress of your pull request and offering the latest feedback. If you encounter any challenges during development, seeking support within the community is encouraged. We sincerely appreciate your contributions to Apache EventMesh. |
Fixes #3402
Motivation
Using requireNonNull() to check incoming references, can control the point in time when the exception will be thrown.
Modifications
Included "Object.requireNonNull()"
Documentation
No
No