[core] Fix checkstyle for Core miscellaneous #901

risdenk · 2017-01-18T20:52:22Z

Relates to #895

manolama

LGTM, just one important revert for compatibility and a couple of nit-picks about private vs protected. Only the DB properties var really needs addressing. The others shouldn't really affect anyone.

manolama · 2017-01-30T02:29:12Z

core/src/main/java/com/yahoo/ycsb/BasicDB.java

    @Override
    protected StringBuilder initialValue() {
      return new StringBuilder();
    }
  };

-  static StringBuilder getStringBuilder() {
+  private static StringBuilder getStringBuilder() {


Super nit-picky but if anyone had their own code that was calling this from within the package or their own sublass then it would be a breaking change. Probably not a big deal at all.

Also would increment the copyright :)

manolama · 2017-01-30T02:33:55Z

core/src/main/java/com/yahoo/ycsb/DB.java

+  /**
+   * Properties for configuring this DB.
+   */
+  private Properties properties = new Properties();


This one needs stay the same _p name so implementers don't see their code break. And protected instead of private.

We can't leave _p since that is not an allowed variable name by checkstyle. I don't see a way around this.

We can provide a custom checkstyle config with the project. It's annoying but better to maintain compatibility than worry about styling.

Ok thats fair I'll see what I can do. I think there is a Suppress annotation way to handle just this line.

So looking at this closer, there already exist get/set property methods so that should encapsulate the _p variable and allow us to change the name. If people were relying on the _p variable they should have been using the get/set methods.

Hmm, yeah I guess since none of the existing DB's use it directly and it's a tiny API change it'd be fine. We should just make a big note of it in the release notes.

manolama · 2017-01-30T02:35:01Z

core/src/main/java/com/yahoo/ycsb/DB.java

+   * @param result A HashMap of field/value pairs for the result
+   * @return The result of the operation.
+   */
+  public abstract Status read(String table, String key, Set<String> fields, HashMap<String, ByteIterator> result);


It might be nice to change the HashMap to a Map since that shouldn't break the API and be consistent with Set.

I'm going to leave this to another issue. This requires changing all the implementations to match the method signature of the abstract method.

manolama · 2017-01-30T02:38:58Z

core/src/main/java/com/yahoo/ycsb/GoodBadUglyDB.java

+  public static final String SIMULATE_DELAY = "gbudb.delays";
+  public static final String SIMULATE_DELAY_DEFAULT = "200,1000,10000,50000,100000";
+  private long[] delays;
+  private static final ReadWriteLock DB_ACCESS = new ReentrantReadWriteLock();


Nother nit regarding the privatization of these two. Shouldn't be anyone using them but may be good to set them to protected just to be safe.

manolama · 2017-01-30T02:40:14Z

core/src/main/java/com/yahoo/ycsb/InputStreamByteIterator.java

-	}
+  private long len;
+  private InputStream ins;
+  private long off;


Nother nit, maybe protected just to be safe.

manolama · 2017-01-30T02:40:52Z

core/src/main/java/com/yahoo/ycsb/StringByteIterator.java

-	String str;
-	int off;
+  private String str;
+  private int off;


nother nit, protected

manolama · 2017-01-30T02:41:46Z

core/src/main/java/com/yahoo/ycsb/package-info.java

@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2015 - 2016 YCSB contributors. All rights reserved.


2015 - 2017 :)

risdenk · 2017-01-30T02:50:10Z

@manolama Thanks for the review! I'll update it tomorrow most likely.

risdenk · 2017-01-30T15:44:10Z

@manolama Updated the headers and fixed the getStringBuilder method to be protected.

A few of the changes I didn't make since they made checkstyle very unhappy.

private -> protected variables - this complains that they should be private with getters/setters. I can add getters/setters but figured we could if/when necessary.
properties -> _p - this complains about variable naming and there are already getters/setters so shouldn't be a problem changing the variable name

The comment about HashMap -> Map

HashMap -> Map - this requires changing all the implementations of the abstract class to match the signature. This would be better to do in a separate issue.

My thought is that we should break compatibility and enforce checkstyle so it can't happen again. Right now we are not enforcing at all which makes it hard to even guarantee compatibility.

risdenk · 2017-01-30T22:02:39Z

Thanks @manolama!

risdenk self-assigned this Jan 18, 2017

risdenk requested a review from busbey January 18, 2017 20:52

risdenk added the improvement label Jan 19, 2017

risdenk modified the milestone: 0.13.0 Jan 19, 2017

risdenk mentioned this pull request Jan 30, 2017

[core] Add the NumericByteIterator for dealing with numeric values. #912

Closed

manolama requested changes Jan 30, 2017

View reviewed changes

risdenk force-pushed the checkstyle-core-miscellaneous branch from 9007cd8 to 5bada1e Compare January 30, 2017 15:37

[core] Fix checkstyle for Core miscellaneous

90ab752

risdenk force-pushed the checkstyle-core-miscellaneous branch from 5bada1e to 90ab752 Compare January 30, 2017 15:39

manolama approved these changes Jan 30, 2017

View reviewed changes

risdenk merged commit 87d398f into brianfrankcooper:master Jan 30, 2017

risdenk deleted the checkstyle-core-miscellaneous branch January 30, 2017 22:02

tzm41 pushed a commit to tzm41/YCSB that referenced this pull request May 7, 2017

[core] Fix checkstyle for Core miscellaneous (brianfrankcooper#901)

04b960b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Fix checkstyle for Core miscellaneous #901

[core] Fix checkstyle for Core miscellaneous #901

risdenk commented Jan 18, 2017

manolama left a comment

manolama Jan 30, 2017

manolama Jan 30, 2017

manolama Jan 30, 2017

risdenk Jan 30, 2017

manolama Jan 30, 2017

risdenk Jan 30, 2017

risdenk Jan 30, 2017

manolama Jan 30, 2017

manolama Jan 30, 2017

risdenk Jan 30, 2017

manolama Jan 30, 2017

manolama Jan 30, 2017

manolama Jan 30, 2017

manolama Jan 30, 2017

risdenk commented Jan 30, 2017

risdenk commented Jan 30, 2017

risdenk commented Jan 30, 2017

		@@ -0,0 +1,22 @@
		/*
		* Copyright (c) 2015 - 2016 YCSB contributors. All rights reserved.

[core] Fix checkstyle for Core miscellaneous #901

[core] Fix checkstyle for Core miscellaneous #901

Conversation

risdenk commented Jan 18, 2017

manolama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

risdenk commented Jan 30, 2017

risdenk commented Jan 30, 2017

risdenk commented Jan 30, 2017