TEZ-4669: Spotbugs fails in precommit#457
Conversation
|
I was also debugging this 😅. I think based on the stacktrace the issue is because of missing So, if we somehow add that class, it should work. I was going to add |
|
💔 -1 overall
This message was automatically generated. |
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |
|
I think the CI didn't kick in the Spotbugs? |
ack, apparently fake pom.xml change is not enough, let me do a fake .java change |
|
💔 -1 overall
This message was automatically generated. |
|
yikes, the same error, even though it worked for me on ubuntu, need to double-check this |
@abstractdog , can you please give 1 try with the plexus-velocity dependency? As the |
|
💔 -1 overall
This message was automatically generated. |
|
plexus-velocity solves the problem on my testing ubuntu server, but not in precommit, unfortunately, as a next step, I'm going for the yetus image and doing it inside |
UPDATE: the pom.xml change solves the problem in the yetus docker container too for me locally...this is getting more and more interesting in the failing job I can see: whereas for me locally: apparently spotbugs in precommit doesn't depend on the fix |
Yes, looks like it: Lines 119 to 120 in 03b5f1d So, if we merge this then for new PR's this change should be ok I believe! |
I believe so, btw, not sure how "spotbugs-strict-precheck" is related to this behavior, how did you mean? |
My mistake, I got confused. |
|
I think SpotBugs passed: The patch results: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-457/7/artifact/out/patch-spotbugs-root.txt Am I missing something? |
LOL yes! :) the master branch doesn't contain the patch, that's the point |
|
so with the patch: @ayushtkn : could this be merged now? |
ayushtkn
left a comment
There was a problem hiding this comment.
LGTM.
One comment, feel free to merge once you change
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.codehaus.plexus</groupId> | ||
| <artifactId>plexus-velocity</artifactId> | ||
| <version>2.3.0</version> |
There was a problem hiding this comment.
Can you define it as a variable and use, all other dependencies have defined variable, I think we did that explicitly in some patch in past to define variable
There was a problem hiding this comment.
hm, I was hesitating to do that: it's just a workaround that ideally should go away with a spotbugs upgrade, does it "deserve" a top-level property?
There was a problem hiding this comment.
I would say lets keep the code in sync, once we upgrade and it is no longer required we can drop it like a normal dependency, it is just one more line to rollback and keep the code in sync
There was a problem hiding this comment.
ack, it won't hurt :)
please see bef9055
assuming your approval holds with this change, I'm waiting for precommit tests
This comment was marked as outdated.
This comment was marked as outdated.
|
💔 -1 overall
This message was automatically generated. |


Upgraded sisu-inject-plexus that spotbugs plugin depends on to make it work (it was mainly a guesswork).
Regarding the process:
solution double-checked locally too: