-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Improve unit tests performance (CASSANDRA-17427 --> trunk) #1488
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
Improve unit tests performance (CASSANDRA-17427 --> trunk) #1488
Conversation
b77b5b9 to
d81de53
Compare
8969b49 to
b09e3cf
Compare
|
CI ci-cassandra: https://ci-cassandra.apache.org/job/Cassandra-devbranch/1834/ |
1f56674 to
fa13ab6
Compare
591ab5b to
7deceda
Compare
jmckenzie-dev
left a comment
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.
No really major issues on the review; looking solid.
src/java/org/apache/cassandra/config/CassandraRelevantProperties.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.
Should this be a CassandraRelevantProperties?
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.
@josh-mckenzie great catch, yes, it definitely should be there. If we managed to have the second reviewer for CASSANDRA-17797 and we merged that work, this would fail the build :)
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.
Quite interesting question. This is something to be used only in tests. Perhaps something overly contrived but maybe test properties should require something like cassandra.test property to be set in order to make them get assigned with a non-default value? (I mean, set cassandra.test to unlock those properties). Perhaps something for CASSANDRA-17797 @smiklosovic
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 patch we work on is gathering all properties, irrelevant what they are for, in one place. I understand these are just test properties but having them somewhere separately, for example, in test source, would mean that we would start to reference test class in the prod code. Right? Not good. Having test code referencing prod code where these test properties are is better.
I am not sure how to reply to your idea about cassandra.test. I think that in general it is good idea to have them with that prefix, yes. Otherwise we are not completely sure, at the first glance, if there is not any comment, what that property is actually used for / upon.
In the long run, we would like to have all test properties with such prefix (it is really messy right now) but the first task is to manage the current "chaos" with having them scattered across all the source code. As of now, nobody has a definitive answer what all properties we use ....
I think test properties with some value should be set as the very first thing in the test class? So the defaults would be overridden (if desirable) by a tester upon starting that test when this is final static. If the value of that property is meant to be changed during the test then this needs to be fixed on the code level to resolve that property dynamically as it is called.
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 just meant to prevent setting those properties to some non-default values in production, that is, you can only override the defaults if some additional property like, -Dcassandra.test=true is set
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.
ah right ... @Mmuzaf this is an interesting point to take into account, we might deal with this later I guess.
test/distributed/org/apache/cassandra/distributed/impl/AbstractCluster.java
Outdated
Show resolved
Hide resolved
|
In general there's a lot of whitespace removal in various files; I'm not sure why we have so many files w/trailing whitespace and we don't have anything in our style guide about it that I can see (link). So is without or with the way to go? |
|
@josh-mckenzie I've applied your review comments, please have a look before I rebase and overwrite the branch |
a139dce to
21c6a8d
Compare
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.
@jacek-lewandowski this should be probably undeleted.
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'll make sure there is no such changes before committing/merging
test/distributed/org/apache/cassandra/distributed/impl/AbstractCluster.java
Outdated
Show resolved
Hide resolved
21c6a8d to
861316e
Compare
ea81d88 to
be40038
Compare
49c0125 to
7d28b97
Compare
acdecca to
045878e
Compare
- Don't flush schema on every schema update in unit tests - Use unix command to delete test data - Shorten teardown - Stable processor count presented by JMX on Jenkins, CircleCI and local Patch by <jacek-lewandowski>, reviewed by <michaelsembwever> and <josh-mckenzie> for CASSANDRA-17427
045878e to
6d90d30
Compare
Do all coordinator-side sorting for index queries in StorageAttachedIndexQueryPlan#postProcessor.
Do all coordinator-side sorting for index queries in StorageAttachedIndexQueryPlan#postProcessor.
Do all coordinator-side sorting for index queries in StorageAttachedIndexQueryPlan#postProcessor.
patch by @jacek-lewandowski reviewed by @michaelsembwever and @josh-mckenzie for CASSANDRA-17427