Re: [DISCUSSION] Add SpotBugs to the build
Hi David, all what is the position of Jacoco in Cassandra as this point? I see it in build.xml there are some targets and infrastructure around that. What is wrong with using Jacoco instead more heavily for static analysis? Why do we need to introduce this? Regards From: David Capwell Sent: Tuesday, November 8, 2022 0:10 To: dev Subject: [DISCUSSION] Add SpotBugs to the build NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe. I was thinking that it would be good to add SpotBugs (https://spotbugs.github.io) into our build to help find bugs earlier in the life cycle. SpotBugs is LGPL but as it is used only in the build and not to within this project, then this should be fine with Apache. The motivation for adding this was from CASSANDRA-17178; the Simulator has issues with Serializable classes missing serialVersionUID (as we deal with ClassLoaders; this field is strongly recommend in general for all Serializable classes), but this project can add more value as there are a large collection of potential bugs to look out for; below are a few examples found. * Number.valueOf vs Number.parse. In many parts of the code we do valueOf which returns a boxed value; we then unbox for the usage; this adds more garbage that isn’t needed * Using Number.compareTo rather than primitive compare functions (causing boxing) * Ignoring return value for functions that don’t have a side effect. This happens in a few cases where we are building a StringBuilder where we call .toString but ignore the string… then call it later on * use of putIfAbsent without looking at the return. This was found in CacheService where we add the SSTable reader to the cache and assume we win the race and start using it… rather than using the object that won the race
Re: [DISCUSSION] Add SpotBugs to the build
I confused Jacoco to be code analysis tool instead of code coverage one. Early morning I guess :) I was thinking more about tooling like Sonar. What do you think about that? Any pros / cons to Spotbugs? From: Miklosovic, Stefan Sent: Tuesday, November 8, 2022 11:36 To: dev@cassandra.apache.org Subject: Re: [DISCUSSION] Add SpotBugs to the build Hi David, all what is the position of Jacoco in Cassandra as this point? I see it in build.xml there are some targets and infrastructure around that. What is wrong with using Jacoco instead more heavily for static analysis? Why do we need to introduce this? Regards From: David Capwell Sent: Tuesday, November 8, 2022 0:10 To: dev Subject: [DISCUSSION] Add SpotBugs to the build NetApp Security WARNING: This is an external email. Do not click links or open attachments unless you recognize the sender and know the content is safe. I was thinking that it would be good to add SpotBugs (https://spotbugs.github.io) into our build to help find bugs earlier in the life cycle. SpotBugs is LGPL but as it is used only in the build and not to within this project, then this should be fine with Apache. The motivation for adding this was from CASSANDRA-17178; the Simulator has issues with Serializable classes missing serialVersionUID (as we deal with ClassLoaders; this field is strongly recommend in general for all Serializable classes), but this project can add more value as there are a large collection of potential bugs to look out for; below are a few examples found. * Number.valueOf vs Number.parse. In many parts of the code we do valueOf which returns a boxed value; we then unbox for the usage; this adds more garbage that isn’t needed * Using Number.compareTo rather than primitive compare functions (causing boxing) * Ignoring return value for functions that don’t have a side effect. This happens in a few cases where we are building a StringBuilder where we call .toString but ignore the string… then call it later on * use of putIfAbsent without looking at the return. This was found in CacheService where we add the SSTable reader to the cache and assume we win the race and start using it… rather than using the object that won the race
Re: [DISSCUSS] Access to JDK internals only after dev mailing list consensus?
Left a couple thoughts that are probably outside the scope of this work but fast-follows, and one nit on ordering (hit ML before documenting on JIRA vs. hit JIRA and then hit up ML). Otherwise, looks great! On Mon, Nov 7, 2022, at 2:56 PM, Chen-Becker, Derek wrote: > There was one very minor grammatical nit that I commented on, but I think > that otherwise this is clear and well-written 😊 > > Cheers, > > Derek > > > *From: *Ekaterina Dimitrova > *Reply-To: *"dev@cassandra.apache.org" > *Date: *Friday, November 4, 2022 at 3:51 PM > *To: *"dev@cassandra.apache.org" > *Subject: *RE: [EXTERNAL][DISSCUSS] Access to JDK internals only after dev > mailing list consensus? > > *CAUTION*: This email originated from outside of the organization. Do not > click links or open attachments unless you can confirm the sender and know > the content is safe. > > > 👋 > > I finally got the chance to put down a proposal for a section at the end of > the Cassandra Code Style document. > Please help a fellow non-native speaker and definitely not a writer with some > constructive criticism. :-) > My proposal is in this commit - > https://github.com/ekaterinadimitrova2/cassandra-website/commit/4a9edc7e88fd9fc2c6aa8a84290b75b02cac03bf > > I noticed the Dependencies section suggested in the past by Benedict was > missing, even if we had a consensus around that. I added it back from the > original doc. > > Best regards , > Ekaterina > > On Fri, 9 Sep 2022 at 13:34, Ekaterina Dimitrova > wrote: >> Hi everyone, >> Seems to me that people will be fine with heads up on the mailing list and >> details on tickets? >> Plus update of the Code Style to add a point around that, as suggested. >> >> I will leave this thread open a few more days and if there are no objections >> I will continue with documenting it. >> >> Have a great weekend everyone! >> >> On Fri, 2 Sep 2022 at 14:01, Ekaterina Dimitrova >> wrote: >>> Git and jira , nothing specific >>> >>> On Fri, 2 Sep 2022 at 12:51, Derek Chen-Becker >>> wrote: I think it's fine to state it explicitly rather than making it an assumption. Are we tracking any usage of internals in the codebase currently? Cheers, Derek On Fri, Sep 2, 2022 at 6:30 AM Ekaterina Dimitrova wrote: > > > “ A quick heads up to the dev list with the jira would be sufficient for > anybody interested in discussing it further to comment on the jira.” > > Agreed, I did’t mean voting but more or less we have the lazy consensus > or sharing concerns. Discussing them on a ticket should be enough but it > needs to happen. Also, it shouldn’t be more often than adding > dependencies I guess. > > JDK team is only closing more and more internals and warning us about > potential breakages. I want to prevent us from urgent fixing in patch > releases and to ease the maintenance. > > I think ensuring that it is clearly documented why an exception is > acceptable and what options were considered will be of benefit for > maintenance. We can revise in time what has changed. > > “ . Unless absolutely needed we should avoid accessing the internals. > Folks on this project should understand why. We can make the dangers of > this explicit in our contributor documentation. ” > +1 > > On Fri, 2 Sep 2022 at 1:26, Dinesh Joshi wrote: >> Personally not opposed to this. However, this is something that should >> be vetted closely by the reviewers. Unless absolutely needed we should >> avoid accessing the internals. Folks on this project should understand >> why. We can make the dangers of this explicit in our contributor >> documentation. However, requiring an entire dev list discussion around >> it seems unnecessary. A quick heads up to the dev list with the jira >> would be sufficient for anybody interested in discussing it further to >> comment on the jira. WDYT? >> Dinesh >> >> >>> On Sep 1, 2022, at 8:31 AM, Ekaterina Dimitrova >>> wrote: >>> Hi everyone, >>> >>> Some time ago we added a note to the project Cassandra Code Style: >>> “New dependencies should not be included without community consensus >>> first being obtained via a [DISCUSS] thread on the >>> dev@cassandra.apache.org mailing list” >>> >>> I would like to suggest also to add a point around accessing JDK >>> internals. Any patch that suggests accessing internals and/or adding >>> even more add-opens/add-exports to be approved prior commit on the >>> mailing list. >>> >>> It seems to me the project can only benefit of this visibility. If >>> something is accepted as an exception, we need to have the right >>> understanding and visibility of why; in some cases maybe to see for >>> alternatives, to have f
Re: Cassandra project status update 2022-11-07
Thanks Stefan; I added the 4.1-rc fixver to the ticket we already have. Could you do the same when you create a ticket for that other failure? On Mon, Nov 7, 2022, at 5:29 PM, Miklosovic, Stefan wrote: > Hi Josh, > > thanks for the status. > > I would like to raise awareness that as we fix CASSANDRA-17964, it will > introduce two tests which will start to fail (because they were not executed > as part of CI until now because how they are named (not ending on *Test)). > > I believe that these tests will need to be addressed and fixed before 4.1 is > out. > > My email describing that in more detail is here (1). > > (1) https://lists.apache.org/thread/pl0q1krhgv0rvybp5jmdy3411hchy28l > > Regards, > > Stefan > > (1) https://lists.apache.org/thread/pl0q1krhgv0rvybp5jmdy3411hchy28l > > > From: Josh McKenzie > Sent: Monday, November 7, 2022 22:59 > To: dev > Subject: Cassandra project status update 2022-11-07 > > NetApp Security WARNING: This is an external email. Do not click links or > open attachments unless you recognize the sender and know the content is safe. > > > > Oh good grief, it's been 26 days since I wrote one of these. My apologies! > (Life happens - I can confirm that the terribly named "triple-demic" is real > folks) > > We've had a number of releases since the last status email. The current and > latest supported GA cassandra releases across all branches are: > > - cassandra 4: 4.0.7 > - cassandra 3.11: 3.11.14 > - cassandra 3.0: 3.0.28 > > > [Needs Committers] > I'd like to first focus our attention on tickets that are flagged as "Needs > Committer". Our project rules for Cassandra are that 2 committers need to > sign off on a commit, so many times if an author or reviewer isn't yet a > committer, these tickets can need external input to get into the codebase. > The following URL is for a query to pull the Needs Committer tickets: > https://issues.apache.org/jira/issues/?jql=project%20%3D%20CASSANDRA%20and%20resolution%20%3D%20unresolved%20and%20status%20%3D%20%22Needs%20Committer%22 > > CASSANDRA-17861, Update Python test framework from nose to pytest in CCM > could use another committer on it: > https://issues.apache.org/jira/browse/CASSANDRA-17861 > > CASSANDRA-17870, nodetool/rebuild: Add flag to exclude nodes from local > datacenter could also use another committer on review: > https://issues.apache.org/jira/browse/CASSANDRA-17870 > > CASSANDRA-15402, Make incremental backup configurable per keyspace and table > looks like it has committer attention as per a recent comment so we're good > there. > > CASSANDRA-14930, decommission may cause timeout because messaging backlog is > cleared: not sure why this one is marked as Needs Committer actually as it > has 2 as reviewer. Might just need a status update. > > Before we get to 4.1 status, I'd like to call out that Trie memtables were > merged in CASSANDRA-17240. This is a large body of novel work (that Branimir > presented on at ApacheCon for those of you lucky enough to attend) and it's > great to see this land in the project; it's worth your time to pop open that > diff and take a look around and see some of the new things being added to > Cassandra. Notably, there's some great discussion about property-based > testing going on in the comments which has sparked some offline discussion > about how we can integrate exploratory fuzz testing in our primary CI > pipeline; more to come on that front as discussions evolve. > > > [4.1 status] > Let's move on to 4.1 status. We're down to 2 tickets blocking rc, and I'm > given to understand that the one in progress is close to having something to > review, so on the "outstanding work" side we're in great shape: > https://issues.apache.org/jira/secure/RapidBoard.jspa?rapidView=484 > > That leaves us with the question: what do we do about CI? We've recently > expanded our governance options as to what we consider validated and cleared > for release: > https://cwiki.apache.org/confluence/display/CASSANDRA/Release+Lifecycle. > Specifically: > > "Three consecutive green runs of circleci OR of ASF CI are required to cut RC" > > Our most recent run of 4.1 on ASF infra had 9 failures - > https://butler.cassandra.apache.org/#/ci/upstream/compare/Cassandra-4.1/cassandra-4.1. > This has been trending up a bit very recently from a low of 1 a bit over a > week ago; the lion's share of the failures look to be environmental with > timeouts. > > With ASF CI having stragglers that are flaking lately, option 2 would be > three consecutive green runs on circleci, however in order for this to be > representative we need some improvements to the test configuration in circle > to get it into parity with the ASF env, as tracked in CASSANDRA-17930 here: > https://issues.apache.org/jira/browse/CASSANDRA-17930. As of a recent comment > Ekaterina's taking point on this and tracking that addition in > CASSANDRA-18
Re: [DISCUSSION] Add SpotBugs to the build
We used to do linting years ago, and our approach was basically "the past is past, but look for deltas of new bad things added and notify the author". At least for me personally the signal / noise ratio was quite valuable (and not just for my own code; i.e. this wasn't *just* a "Josh Problem" ;) ) I was thinking about this w/the checkstyle additions; we need the ability to bypass these easily for incremental change / test cycles while working on something so devs don't have to pay the "analyze the entire code-base" tax on each small local change. I'm a strong +1 on having these tools in here and on by default for build or jar targets. Maybe it'd be more friendly to have something like "ant unsafe" (or "ant jar-unsafe" so it's adjacent) that just does a quick jar w/out that stuff so it's more discoverable than multiple -D params? On Tue, Nov 8, 2022, at 5:40 AM, Miklosovic, Stefan wrote: > I confused Jacoco to be code analysis tool instead of code coverage one. > Early morning I guess :) I was thinking more about tooling like Sonar. What > do you think about that? Any pros / cons to Spotbugs? > > > From: Miklosovic, Stefan > Sent: Tuesday, November 8, 2022 11:36 > To: dev@cassandra.apache.org > Subject: Re: [DISCUSSION] Add SpotBugs to the build > > Hi David, all > > what is the position of Jacoco in Cassandra as this point? I see it in > build.xml there are some targets and infrastructure around that. > > What is wrong with using Jacoco instead more heavily for static analysis? Why > do we need to introduce this? > > Regards > > > From: David Capwell > Sent: Tuesday, November 8, 2022 0:10 > To: dev > Subject: [DISCUSSION] Add SpotBugs to the build > > NetApp Security WARNING: This is an external email. Do not click links or > open attachments unless you recognize the sender and know the content is safe. > > > > > I was thinking that it would be good to add SpotBugs > (https://spotbugs.github.io) into our build to help find bugs earlier in the > life cycle. SpotBugs is LGPL but as it is used only in the build and not to > within this project, then this should be fine with Apache. > > The motivation for adding this was from CASSANDRA-17178; the Simulator has > issues with Serializable classes missing serialVersionUID (as we deal with > ClassLoaders; this field is strongly recommend in general for all > Serializable classes), but this project can add more value as there are a > large collection of potential bugs to look out for; below are a few examples > found. > > * Number.valueOf vs Number.parse. In many parts of the code we do > valueOf which returns a boxed value; we then unbox for the usage; this adds > more garbage that isn’t needed > * Using Number.compareTo rather than primitive compare functions (causing > boxing) > * Ignoring return value for functions that don’t have a side effect. This > happens in a few cases where we are building a StringBuilder where we call > .toString but ignore the string… then call it later on > * use of putIfAbsent without looking at the return. This was found in > CacheService where we add the SSTable reader to the cache and assume we win > the race and start using it… rather than using the object that won the race >
Re: [DISCUSSION] Add SpotBugs to the build
"ant skip-code-checks" might be more discoverable (and less intimidating) than "ant unsafe". I'm a +100 on enabling more static analysis/linting/coverage reporting for full builds. On Tue, Nov 8, 2022, 6:42 AM Josh McKenzie wrote: > We used to do linting years ago, and our approach was basically "the past > is past, but look for deltas of new bad things added and notify the > author". At least for me personally the signal / noise ratio was quite > valuable (and not just for my own code; i.e. this wasn't *just* a "Josh > Problem" ;) ) > > I was thinking about this w/the checkstyle additions; we need the ability > to bypass these easily for incremental change / test cycles while working > on something so devs don't have to pay the "analyze the entire code-base" > tax on each small local change. I'm a strong +1 on having these tools in > here and on by default for build or jar targets. > > Maybe it'd be more friendly to have something like "ant unsafe" (or "ant > jar-unsafe" so it's adjacent) that just does a quick jar w/out that stuff > so it's more discoverable than multiple -D params? > > On Tue, Nov 8, 2022, at 5:40 AM, Miklosovic, Stefan wrote: > > I confused Jacoco to be code analysis tool instead of code coverage one. > Early morning I guess :) I was thinking more about tooling like Sonar. What > do you think about that? Any pros / cons to Spotbugs? > > > From: Miklosovic, Stefan > Sent: Tuesday, November 8, 2022 11:36 > To: dev@cassandra.apache.org > Subject: Re: [DISCUSSION] Add SpotBugs to the build > > Hi David, all > > what is the position of Jacoco in Cassandra as this point? I see it in > build.xml there are some targets and infrastructure around that. > > What is wrong with using Jacoco instead more heavily for static analysis? > Why do we need to introduce this? > > Regards > > > From: David Capwell > Sent: Tuesday, November 8, 2022 0:10 > To: dev > Subject: [DISCUSSION] Add SpotBugs to the build > > NetApp Security WARNING: This is an external email. Do not click links or > open attachments unless you recognize the sender and know the content is > safe. > > > > > I was thinking that it would be good to add SpotBugs ( > https://spotbugs.github.io) into our build to help find bugs earlier in > the life cycle. SpotBugs is LGPL but as it is used only in the build and > not to within this project, then this should be fine with Apache. > > The motivation for adding this was from CASSANDRA-17178; the Simulator has > issues with Serializable classes missing serialVersionUID (as we deal with > ClassLoaders; this field is strongly recommend in general for all > Serializable classes), but this project can add more value as there are a > large collection of potential bugs to look out for; below are a few > examples found. > > * Number.valueOf vs Number.parse. In many parts of the code we do > valueOf which returns a boxed value; we then unbox for the usage; this adds > more garbage that isn’t needed > * Using Number.compareTo rather than primitive compare functions (causing > boxing) > * Ignoring return value for functions that don’t have a side effect. This > happens in a few cases where we are building a StringBuilder where we call > .toString but ignore the string… then call it later on > * use of putIfAbsent without looking at the return. This was found in > CacheService where we add the SSTable reader to the cache and assume we win > the race and start using it… rather than using the object that won the race > > >
Re: [DISCUSSION] Add SpotBugs to the build
Fair point on the name both being more representative of what it does and being less scary. Wish we could think of something alpha-adjacent to "ant jar" so folks that are thinking "I want to build this thing" immediately see both options; the ant -p output is getting pretty long. On Tue, Nov 8, 2022, at 9:35 AM, Derek Chen-Becker wrote: > "ant skip-code-checks" might be more discoverable (and less intimidating) > than "ant unsafe". I'm a +100 on enabling more static > analysis/linting/coverage reporting for full builds. > > On Tue, Nov 8, 2022, 6:42 AM Josh McKenzie wrote: >> __ >> We used to do linting years ago, and our approach was basically "the past is >> past, but look for deltas of new bad things added and notify the author". At >> least for me personally the signal / noise ratio was quite valuable (and not >> just for my own code; i.e. this wasn't *just* a "Josh Problem" ;) ) >> >> I was thinking about this w/the checkstyle additions; we need the ability to >> bypass these easily for incremental change / test cycles while working on >> something so devs don't have to pay the "analyze the entire code-base" tax >> on each small local change. I'm a strong +1 on having these tools in here >> and on by default for build or jar targets. >> >> Maybe it'd be more friendly to have something like "ant unsafe" (or "ant >> jar-unsafe" so it's adjacent) that just does a quick jar w/out that stuff so >> it's more discoverable than multiple -D params? >> >> On Tue, Nov 8, 2022, at 5:40 AM, Miklosovic, Stefan wrote: >>> I confused Jacoco to be code analysis tool instead of code coverage one. >>> Early morning I guess :) I was thinking more about tooling like Sonar. What >>> do you think about that? Any pros / cons to Spotbugs? >>> >>> >>> From: Miklosovic, Stefan >>> Sent: Tuesday, November 8, 2022 11:36 >>> To: dev@cassandra.apache.org >>> Subject: Re: [DISCUSSION] Add SpotBugs to the build >>> >>> Hi David, all >>> >>> what is the position of Jacoco in Cassandra as this point? I see it in >>> build.xml there are some targets and infrastructure around that. >>> >>> What is wrong with using Jacoco instead more heavily for static analysis? >>> Why do we need to introduce this? >>> >>> Regards >>> >>> >>> From: David Capwell >>> Sent: Tuesday, November 8, 2022 0:10 >>> To: dev >>> Subject: [DISCUSSION] Add SpotBugs to the build >>> >>> NetApp Security WARNING: This is an external email. Do not click links or >>> open attachments unless you recognize the sender and know the content is >>> safe. >>> >>> >>> >>> >>> I was thinking that it would be good to add SpotBugs >>> (https://spotbugs.github.io) into our build to help find bugs earlier in >>> the life cycle. SpotBugs is LGPL but as it is used only in the build and >>> not to within this project, then this should be fine with Apache. >>> >>> The motivation for adding this was from CASSANDRA-17178; the Simulator has >>> issues with Serializable classes missing serialVersionUID (as we deal with >>> ClassLoaders; this field is strongly recommend in general for all >>> Serializable classes), but this project can add more value as there are a >>> large collection of potential bugs to look out for; below are a few >>> examples found. >>> >>> * Number.valueOf vs Number.parse. In many parts of the code we do >>> valueOf which returns a boxed value; we then unbox for the usage; this adds >>> more garbage that isn’t needed >>> * Using Number.compareTo rather than primitive compare functions (causing >>> boxing) >>> * Ignoring return value for functions that don’t have a side effect. This >>> happens in a few cases where we are building a StringBuilder where we call >>> .toString but ignore the string… then call it later on >>> * use of putIfAbsent without looking at the return. This was found in >>> CacheService where we add the SSTable reader to the cache and assume we win >>> the race and start using it… rather than using the object that won the race >>> >>
Re: [DISCUSSION] Add SpotBugs to the build
Fair point on the name both being more representative of what it does and > being less scary. Wish we could think of something alpha-adjacent to "ant > jar" so folks that are thinking "I want to build this thing" immediately > see both options; the ant -p output is getting pretty long. > What's the saying when the fish can't help but bite that bait… the patch in my last post, it introduces aggregate skip flags, like `-Dno-linter=` and `-Dno-docs=`, so it would also be easy to introduce a top-level cheat like `-Dfast=` and you'd get this with `ant jar -Dfast=`
Re: [DISCUSSION] Add SpotBugs to the build
Thanks all for the replies, I hope I am posting a summary of all the feedback 1) double check with legal due to LGPL 2) need way to opt-out during development similar to checkstyle 3) patch should be in the same spirit as the other build changes, hiding details in .build and following naming 4) cool with this idea, but why not X instead? 5) rather than processing all the code, can we do a delta instead? For #2/#3 strongly agree, we should not reinvent the wheel and should follow norms (even if they are new or slowly developing as we enhance build.xml). If you wish to opt-out during development, that’s totally fine by me, though strongly feel we should guard in CI. #4, I am cool with others if they solve the problem I had, detecting lack of serialize version UID for serializable classes; this is an issue with simulator so want to make sure the tool solves this problem for us. I looked into error-prone and found its checks very limiting and lacking this check…. So if you know of a checker that solves the serialize version UID issue, I am open to switching #5. The issue I have with delta is that we need to start building “smarts” into our ant build to detect what has changed (similar to other build tools), this looks to be more work than I am willing to put into this work, but I am cool if someone else would be willing to enhance our build to be less wasteful. > On Nov 8, 2022, at 7:48 AM, Mick Semb Wever wrote: > > > > Fair point on the name both being more representative of what it does and > being less scary. Wish we could think of something alpha-adjacent to "ant > jar" so folks that are thinking "I want to build this thing" immediately see > both options; the ant -p output is getting pretty long. > > > What's the saying when the fish can't help but bite that bait… > > the patch in my last post, it introduces aggregate skip flags, like > `-Dno-linter=` and `-Dno-docs=`, so it would also be easy to introduce a > top-level cheat like `-Dfast=` and you'd get this with > > `ant jar -Dfast=` > >
New episode of The Apache Cassandra(R) Corner
Link to next episode: Ep13 - Alexa Simon (Realeyes Sr. Data Engineer) https://drive.google.com/file/d/17vVbAirAJABH4ZbNS-4Yk-eeUbQCyTWd/view?usp=share_link (You may have to download it to listen) It will remain in staging for 72 hours, going live (assuming no objections) by Friday (afternoon), November 11th. If anyone should have any questions, comments, or if you want to be a guest, please reach out to me. Thanks, everyone! Aaron
