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.

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

Issue 923 #940

Merged
merged 4 commits into from
Oct 17, 2017
Merged

Issue 923 #940

merged 4 commits into from
Oct 17, 2017

Conversation

jkosh44
Copy link
Contributor

@jkosh44 jkosh44 commented Oct 11, 2017

No description provided.

  default ScannerBuilder over(Bytes row) {...}
  default ScannerBuilder over(CharSequence row) {...}
  default ScannerBuilder over(Bytes row, Column col){...}
  default ScannerBuilder over(CharSequence row, , Column col){...}

  default ScannerBuilder overPrefix(Bytes row) {...}
  default ScannerBuilder overPrefix(CharSequence row){...}
  default ScannerBuilder overPrefix(Bytes row, Column col){...}
  default ScannerBuilder overPrefix(CharSequence row, , Column col){...}
…prefix() methods

  ScannerIT
  WeakNotificationIT
  WorkerIT
  FluoIT
  LogIT
@@ -30,6 +31,96 @@
*/
ScannerBuilder over(Span span);

/**
*
* @since 1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

When javadoc is rendered, I think it usually puts the since info at the end. As a result, its my personal pref is put the since tag at the end of the javadoc source. Don't need to change if you don't agree.

@@ -464,7 +464,7 @@ public void testRange() throws Exception {
HashSet<Column> columns = new HashSet<>();

CellScanner cellScanner =
tx2.scanner().over(Span.exact(Bytes.of("d00001"))).fetch(new Column("outlink")).build();
tx2.scanner().over(Bytes.of("d00001")).fetch(new Column("outlink")).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could remove Bytes.of and just have over("d00001"). The calls to Bytes.of are from a time when support for Strings didn't exist everywhere in the API. This was a pre-existing problem, does not have to be fixed in this PR.

* @param colPrefix restrict the scanner to data in {@link org.apache.fluo.api.data.Column} that begins with a prefix
* @return self
*/
default ScannerBuilder overPrefix(Bytes rowPrefix, Column colPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change rowPrefix to row.

*
* @since 1.2.0
* @see org.apache.fluo.api.data.Span#prefix(Bytes, Column)
* @param rowPrefix restrict the scanner to data in rows that begins with a prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

This method scans a column prefix within an exact row. So, need to change rowPrefix to row. Also the javadoc should say something like restrict the scanner to data in specified row.

* @since 1.2.0
* @see org.apache.fluo.api.data.Span#prefix(Bytes, Column)
* @param rowPrefix restrict the scanner to data in rows that begins with a prefix
* @param colPrefix restrict the scanner to data in {@link org.apache.fluo.api.data.Column} that begins with a prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Could say restrict scanner to data that begins with specified column prefix

* @param colPrefix restrict the scanner to data in {@link org.apache.fluo.api.data.Column} that begins with a prefix
* @return self
*/
default ScannerBuilder overPrefix(CharSequence rowPrefix, Column colPrefix) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to make same changes for rowPrefix to row

Changed param rowPrefix to row, and updated javadocs for the following methods:
  overPrefix(Bytes, Column){...}
  overPrefix(CharSequence, Column){...}
Removed use of Bytes.of(String) calls within over() calls from FloutIT tests
@@ -463,8 +463,7 @@ public void testRange() throws Exception {

HashSet<Column> columns = new HashSet<>();

CellScanner cellScanner =
tx2.scanner().over(Span.exact(Bytes.of("d00001"))).fetch(new Column("outlink")).build();
CellScanner cellScanner = tx2.scanner().over("d00001").fetch(new Column("outlink")).build();
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a lot nicer.

@keith-turner
Copy link
Contributor

@jkosh44 there is a javadoc error, you can see this in the Travis build. Javadoc is only built on demand. Travis is configured to build it. You can build it locally with the following commands.

mvn clean package javadoc:aggregate
# if builds, then following command can display it
firefox target/site/apidocs/index.html

Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

I think this is good to go when the javadoc err is resolved.

@keith-turner keith-turner merged commit c737df6 into apache:master Oct 17, 2017
@keith-turner
Copy link
Contributor

I restarted the travis build and it passed, so I merged it. Thanks again @jkosh44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants