Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[core] Fix checkstyle for CoreWorkload #897

Merged
merged 1 commit into from
Jan 31, 2017
Merged

[core] Fix checkstyle for CoreWorkload #897

merged 1 commit into from
Jan 31, 2017

Conversation

risdenk
Copy link
Collaborator

@risdenk risdenk commented Jan 18, 2017

Relates to #895

@risdenk risdenk force-pushed the checkstyle-CoreWorkload branch from 515652c to 891da2c Compare January 18, 2017 19:33
@risdenk risdenk self-assigned this Jan 18, 2017
@risdenk risdenk requested a review from busbey January 18, 2017 19:35
@risdenk risdenk force-pushed the checkstyle-CoreWorkload branch 3 times, most recently from f634de6 to 7a3b3c8 Compare January 18, 2017 19:49
@risdenk risdenk mentioned this pull request Jan 18, 2017
Merged
@risdenk risdenk force-pushed the checkstyle-CoreWorkload branch 2 times, most recently from debf1b1 to 65410c0 Compare January 18, 2017 21:51
@risdenk
Copy link
Collaborator Author

risdenk commented Jan 19, 2017

I checked this with: mvn clean validate | grep CoreWorkload and mvn clean test

@risdenk risdenk modified the milestone: 0.13.0 Jan 19, 2017
@risdenk risdenk mentioned this pull request Jan 30, 2017
Closed
manolama
manolama reviewed Jan 30, 2017
View reviewed changes
Copy link
Collaborator

@manolama manolama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just want to maintain the protecteds in the CoreWorkload class since users (like me) may overload it and just tweak a method or two.

core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java Outdated
@@ -105,14 +83,14 @@
*/
public static final String FIELD_COUNT_PROPERTY_DEFAULT = "10";

int fieldcount;
private int fieldcount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the privates should be protecteds for folks overloading the class (like me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these it would make more sense to add public getters/setters. It would break compatibility for now but would be better long term. I can add the getters/setters and push.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thought here is to actually get the property value yourself instead of relying on CoreWorkload to populate the variable.

String table = p.getProperty(CoreWorkload.TABLENAME_PROPERTY, CoreWorkload.TABLENAME_PROPERTY_DEFAULT);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for proper inheritance you don't want to use getters/setters, you want to access the variables directly (plus avoid the function call [which would eventually be inlined, but...])

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thats fair. Let me see what I can do about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way to allow protected variables in checkstyle.xml. It allowed me to set the variables to be protected.

core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java Outdated
@@ -506,14 +483,14 @@ public void init(Properties p) throws WorkloadException {
INSERTION_RETRY_INTERVAL, INSERTION_RETRY_INTERVAL_DEFAULT));
}

public String buildKeyName(long keynum) {
private String buildKeyName(long keynum) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup I'll make this protected.

core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java
@@ -523,7 +500,7 @@ public String buildKeyName(long keynum) {
* Builds a value for a randomly chosen field.
*/
private HashMap<String, ByteIterator> buildSingleValue(String key) {
HashMap<String, ByteIterator> value = new HashMap<String, ByteIterator>();
HashMap<String, ByteIterator> value = new HashMap<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to require JDK7+ for YCSB? Or should the core still maintain JDK5 or 6 compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pom.xml has the maven-compiler-plugin set to 1.7 for source and target. I don't think we are maintaining JDK 6 compatibility anymore. We started trying to add support for JDK 9 already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, sounds good.

core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java Outdated
@@ -653,7 +635,7 @@ public boolean doTransaction(DB db, Object threadstate) {
* Bucket 1 means incorrect data was returned.
* Bucket 2 means null data was returned when some data was expected.
*/
protected void verifyRow(String key, HashMap<String, ByteIterator> cells) {
private void verifyRow(String key, HashMap<String, ByteIterator> cells) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected let overloaders override

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup I'll make this protected.

@risdenk risdenk force-pushed the checkstyle-CoreWorkload branch from 65410c0 to 1d5ba75 Compare January 30, 2017 22:27
@risdenk risdenk force-pushed the checkstyle-CoreWorkload branch from 1d5ba75 to 79fce75 Compare January 31, 2017 16:05
manolama
manolama approved these changes Jan 31, 2017
View reviewed changes
Copy link
Collaborator

@manolama manolama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@risdenk risdenk merged commit a564c4c into brianfrankcooper:master Jan 31, 2017
@risdenk risdenk deleted the checkstyle-CoreWorkload branch January 31, 2017 22:06
@risdenk
Copy link
Collaborator Author

risdenk commented Jan 31, 2017

Thanks @manolama!

tzm41 pushed a commit to tzm41/YCSB that referenced this pull request May 7, 2017
@risdenk @tzm41
[core] Fix checkstyle for CoreWorkload (brianfrankcooper#897)
4a7d5a0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manolama manolama manolama approved these changes

@busbey busbey Awaiting requested review from busbey

Assignees

@risdenk risdenk

Labels
improvement
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
2 participants
@risdenk @manolama