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

Inline Span static methods in Scanner builder #923

Closed
keith-turner opened this issue Sep 18, 2017 · 17 comments
Closed

Inline Span static methods in Scanner builder #923

keith-turner opened this issue Sep 18, 2017 · 17 comments

Comments

@keith-turner
Copy link
Contributor

To build a scanner over a row, something like the following needs to be done.

 try (Snapshot snap = client.newSnapshot()) {
     CellScanner scanner =  snap.scanner().over(Span.prefix("d:")).build();
}

It would be nice to be able to do something like following instead.

 try (Snapshot snap = client.newSnapshot()) {
     CellScanner scanner =  snap.scanner().overPrefix("d:").build();
 }

This fluent style would make writing code in an IDE with code completion more seamless. To accomplish this I think the following methods should be added to ScannerBuilder. The implementations of these methods should call the existing static methods in Span.

interface ScannerBuilder {
  //implementations of following should call corresponding Span.exact() methods
  default ScannerBuilder over(Bytes row) {over(Span.exact(row));}
  default ScannerBuilder over(CharSequence row) {...}
  default ScannerBuilder over(Bytes row, Column col){...}
  default ScannerBuilder over(CharSequence row, , Column col){...}

  //implementations of following should call corresponding Span.prefix() methods
  default ScannerBuilder overPrefix(Bytes row) {over(Span.prefix(row));}
  default ScannerBuilder overPrefix(CharSequence row){...}
  default ScannerBuilder overPrefix(Bytes row, Column col){...}
  default ScannerBuilder overPrefix(CharSequence row, , Column col){...}
}

The java docs for the new methods should have a @since tag and link to the javadoc in the corresponding Span methods.

All existing test should be updated to use the new methods. For example ScannerIT, FluoIT, WorkerIT, and WeakNotificationIT should be updated to use the new methods.

@mingscraft
Copy link

@keith-turner I can have a go on this issue, Thanks

@keith-turner
Copy link
Contributor Author

Sounds good @codeandplay I look forward to your PR. If you have any questions, feel free to ask here.

@keith-turner
Copy link
Contributor Author

@codeandplay did you have any questions about this issue?

@kennethmcfarland
Copy link
Contributor

If this is up for grabs I'd like to take a shot, if not please keep me in mind for the next help wanted as it was very nice working with you.

@mingscraft
Copy link

mingscraft commented Sep 27, 2017 via email

@keith-turner
Copy link
Contributor Author

Thanks @kpm1985 . Looks like its open, if you want to give it a shot.

@kennethmcfarland
Copy link
Contributor

kennethmcfarland commented Sep 28, 2017 via email

@AshB2108
Copy link

AshB2108 commented Oct 5, 2017

Is the issue taken or can I take it ?

@kennethmcfarland
Copy link
Contributor

kennethmcfarland commented Oct 5, 2017 via email

@kennethmcfarland
Copy link
Contributor

kennethmcfarland commented Oct 8, 2017 via email

@jkosh44
Copy link
Contributor

jkosh44 commented Oct 10, 2017

I've seen this pass through a bunch of hands, so I'm gonna hop on line. If this frees up I'd like to give it a shot.

@keith-turner
Copy link
Contributor Author

I am glad to see all of the interest. I am not sure what the best solution is when multiple people would like to work on the same issue. I think the simplest thing to do is just directly ask the person who expressed interest if they are currently working on the issue or plan to shortly. For my part I can keep looking through the code and finding things that need addressing and open more issues. I just opened #938 as another help wanted issue.

I was thinking we could take the approach that Fluo takes and use optimistic locking (first person to get a PR accepted), but that seems a bit anti-social so I vetoed that idea in my head. I think its better to just discuss, although I know that can be a bit annoying sometimes because you have to wait 24 to 48 hours for a response. I am not sure how to speed up the process and make it civil, so personally I would like to see a civil process. If anyone has any suggestions, would be glad to hear them.

@AshB2108 are you currently working on this or do you plan to work on it in the near term?

@jkosh44
Copy link
Contributor

jkosh44 commented Oct 10, 2017

Don't mean to steal this from anyone. Just saying if it opens up again I'd like to work on it.

@keith-turner
Copy link
Contributor Author

Don't mean to steal this from anyone

I hope I didn't give that impression with my last post. I didn't think you were attempting to do that. I was just airing my thoughts based on activity I have seen on multiple issues posted to helpwanted. I was not actually responding to anything in particular on this issue, until the last sentence.

if it opens up again I'd like to work on it.

That is one thing I was trying to determine, is it open?

@jkosh44
Copy link
Contributor

jkosh44 commented Oct 11, 2017

Gotcha, might as well give @AshB2108 some time to respond

@ctubbsii
Copy link
Member

In my personal opinion, I think you can just start. You can always put a link to your personal branch, so others can collaborate with you on what will eventually be just a single pull request to the upstream Apache Fluo repository. Git being a distributed version control system is nice that way.

@ctubbsii
Copy link
Member

ctubbsii commented Oct 12, 2017

As long as you've made a good faith effort to collaborate (including accepting feedback), nobody can fault you for "sniping" a particular bug, in my opinion.

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

No branches or pull requests

7 participants