You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/03/24 16:07:18 UTC

[GitHub] [skywalking] doddi opened a new pull request #8736: added the ignore rule for thread safety

doddi opened a new pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736


   <!--
       โš ๏ธ Please make sure to read this template first, pull requests that don't accord with this template
       maybe closed without notice.
       Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
       Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
   -->
   
   <!-- ==== ๐Ÿ› Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist ๐Ÿ‘‡ ====
   ### Fix <bug description or the bug issue number or bug issue link>
   - [ ] Add a unit test to verify that the fix works.
   - [ ] Explain briefly why the bug exists and how to fix it.
        ==== ๐Ÿ› Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist ๐Ÿ‘† ==== -->
   
   <!-- ==== ๐Ÿ“ˆ Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist ๐Ÿ‘‡ ====
   ### Improve the performance of <class or module or ...>
   - [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
   - [ ] The benchmark result.
   ```text
   <Paste the benchmark results here>
   ```
   - [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
        ==== ๐Ÿ“ˆ Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist ๐Ÿ‘† ==== -->
   
   <!-- ==== ๐Ÿ†• Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist ๐Ÿ‘‡ ====
   ### <Feature description>
   - [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
   - [ ] Update the documentation to include this new feature.
   - [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
   - [ ] If it's UI related, attach the screenshots below.
        ==== ๐Ÿ†• Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist ๐Ÿ‘† ==== -->
   
   - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
   - [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
   
   A request for Sonatype Lift was to remove the THREAD_SAFETY_VIOLATION violation appearing in PRs. This PR adds a configuration setting to stop the comments appearing in future PR's.
   
   An example on demand run without the setting showing the THREAD_SAFETY_VIOLATION https://lift.sonatype.com/results/github.com/apache/skywalking/01FYY2QG9P3AC8AYBFANRH7ZZ7?t=Infer%7CTHREAD_SAFETY_VIOLATION
   
   An example on demand run using this setting https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS (running from this branch)
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] jlstephens89 edited a comment on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
jlstephens89 edited a comment on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078977355


   > https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS?t=ErrorProne%7CSameNameButDifferent
   > 
   > Another question is, this is a common use of `@Slf4j`, why is this showing up?
   > 
   > https://github.com/doddi/skywalking/blob/2a1e480d18605d89b62e12e1a020241133ad5dfa/apm-webapp/src/main/java/org/apache/skywalking/oap/server/webapp/GlobalErrorWebExceptionHandler.java#L34
   
   Mark is on vacation for the next week so myself and the other Lift engineers will take this over.
   
   @wu-sheng @kezhenxu94 Looks like there might be an issue with Lombok and ErrorProne for this particular finding ([details](https://github.com/google/error-prone/issues/2273)). For now I think we should disable this finding as it's more of a suggestion than a true issue. I'm testing the config change out on another branch and then will merge into this one if it works correctly.'
   
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] jlstephens89 edited a comment on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
jlstephens89 edited a comment on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078977355


   > https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS?t=ErrorProne%7CSameNameButDifferent
   > 
   > Another question is, this is a common use of `@Slf4j`, why is this showing up?
   > 
   > https://github.com/doddi/skywalking/blob/2a1e480d18605d89b62e12e1a020241133ad5dfa/apm-webapp/src/main/java/org/apache/skywalking/oap/server/webapp/GlobalErrorWebExceptionHandler.java#L34
   
   Mark is on vacation for the next week so myself and the Lift engineers will take this over.
   
   @wu-sheng @kezhenxu94 Looks like there might be an issue with Lombok and ErrorProne for this particular finding ([details](https://github.com/google/error-prone/issues/2273)). For now I think we should disable this finding as it's more of a suggestion than a true issue. I'm testing the config change out on another branch and then will merge into this one if it works correctly.'
   
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] jlstephens89 commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
jlstephens89 commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079053849


   Added the rule for SameNameButDifferent: https://github.com/apache/skywalking/pull/8736/commits/8835cdfc3a0149b9f0aab050854e7a64086f794d
   
   Test here shows that finding removed: https://lift.sonatype.com/results/github.com/doddi/skywalking/01FZ0G8NAZ7J3WPN59JXRZQ0HK
   
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079846668


   @jlstephens89 Yes, that is gone.
   
   And I found [**subprocess_without_shell_equals_true**, 76 hits](https://lift.sonatype.com/results/github.com/doddi/skywalking/01FZ0G8NAZ7J3WPN59JXRZQ0HK?t=Bandit%7Csubprocess_without_shell_equals_true) and **assert_used**(200+ hits) are just reviewing codes from submodule(UI) part, which should not be reported in this repo. And the error is from `skywalking-ui/node_modules/`, which are the dependencies, not the codes written by SkyWalking UI side. Could you set rules to skip `/skywalking-ui` folder?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079850428


   [JSC_PARSE_ERROR](https://lift.sonatype.com/results/github.com/doddi/skywalking/01FZ0G8NAZ7J3WPN59JXRZQ0HK?t=Closure-Compiler%7CJSC_PARSE_ERROR) is reporting errors of 3rd party license files. I think we either should skip files not `.java` or expected source files, or at least exclude `/dist-material/release-docs`


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078561476


   https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS?t=ErrorProne%7CSameNameButDifferent
   
   Another question is, this is a common use of `@Slf4j`, why is this showing up?
   
   https://github.com/doddi/skywalking/blob/2a1e480d18605d89b62e12e1a020241133ad5dfa/apm-webapp/src/main/java/org/apache/skywalking/oap/server/webapp/GlobalErrorWebExceptionHandler.java#L34


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078558907


   `.muse` is added in https://github.com/apache/skywalking/blob/master/.licenserc.yaml to ignore license header check.
   
   If `.lift`'s files can't have headers(from tech perspective) too, please update `.licenserc.yaml ` file accordingly.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng merged pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736


   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078982452


   This PR seems from @doddi 's personal for repo, are you able to change there?


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] kezhenxu94 commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078814175


   @doddi can you also check [this](https://github.com/apache/skywalking/pull/8725#discussion_r834068197) and other comments in the same PR, I don't think those `SameNameButDifferent`s are positive 


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] jlstephens89 commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
jlstephens89 commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078977355


   > https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS?t=ErrorProne%7CSameNameButDifferent
   > 
   > Another question is, this is a common use of `@Slf4j`, why is this showing up?
   > 
   > https://github.com/doddi/skywalking/blob/2a1e480d18605d89b62e12e1a020241133ad5dfa/apm-webapp/src/main/java/org/apache/skywalking/oap/server/webapp/GlobalErrorWebExceptionHandler.java#L34
   
   @wu-sheng @kezhenxu94 Looks like there might be an issue with Lombok and ErrorProne for this particular finding ([details](https://github.com/google/error-prone/issues/2273)). For now I think we should disable this finding as it's more of a suggestion than a true issue. I'm testing the config change out on another branch and then will merge into this one if it works correctly.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] jlstephens89 commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
jlstephens89 commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079030880


   > This PR seems from @doddi 's personal for repo, are you able to change there?
   
   Yep he granted me permissions before going on vacation 


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1078560250


   Hi, another thing I noticed is this https://lift.sonatype.com/results/github.com/doddi/skywalking/01FYY8BBCACQ1W0HNQ9HWDBNWS?t=ErrorProne%7CMissingSummary
   
   `MissingSummary` covers the generated codes, which are not controlled by us, they are from `protoc` tool. Is there any way to exclude those 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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079852283


   [EqualsGetClass](https://lift.sonatype.com/results/github.com/doddi/skywalking/01FZ0G8NAZ7J3WPN59JXRZQ0HK?t=ErrorProne%7CEqualsGetClass) has listed some warnings according to the book <[Effective Java 3rd Edition ยง10](https://books.google.com/books?id=BIpDDwAAQBAJ)> or other practice you preferred.
   But, personally, I don't feel this is an issue, and also, this code style is from IntelliJ's code generator.
   
   Is there a way we don't have to add `@SuppressWarnings("EqualsGetClass")`? Because it seems pointless, this should be a very developer's personal style. Sub-instance should or should not be treated as equal, is debatable.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [skywalking] wu-sheng commented on pull request #8736: added the ignore rule for thread safety

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #8736:
URL: https://github.com/apache/skywalking/pull/8736#issuecomment-1079850139


   The [NonAtomicVolatileUpdate](https://lift.sonatype.com/results/github.com/doddi/skywalking/01FZ0G8NAZ7J3WPN59JXRZQ0HK?t=ErrorProne%7CNonAtomicVolatileUpdate) is false warning, I think. We have added `@SuppressWarnings("NonAtomicVolatileUpdate")` to the 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.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org