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

Fix incorrect scan range output in getsplits command #2370

Merged
merged 2 commits into from Nov 29, 2021

Conversation

jmark99
Copy link
Contributor

@jmark99 jmark99 commented Nov 29, 2021

In the Accumulo shell, calling getsplits with the verbose option can result in incorret output. It occurs when the tableId of the table
happens to be a single character and there are other tables where the tableId starts with the same character. This results in the output of getsplits displaying splits for the other tables as well.

Closes #2356

In the Accumulo shell, calling getsplits with the verbose option can
result in incorret output. It occurs when the tableId of the table
happens to be a single character and there are other tables where the
tableId starts with the same character. This results in the output of
getsplits displaying splits for the other tables as well.

Closes apache#2356
@EdColeman
Copy link
Contributor

Questions more than a comment.

Did you consider mocking and using a unit test rather than (or in conjunction with) the IT test?
At what point should we consider breaking up the shell server IT test? That test was lengthy as it was - If it was split up, would there be more parallelism?

@Manno15
Copy link
Contributor

Manno15 commented Nov 29, 2021

This PR seems to be a copy of #2369

@jmark99
Copy link
Contributor Author

jmark99 commented Nov 29, 2021

@Manno15 one is for 2.x and the other for 1.x

@Manno15
Copy link
Contributor

Manno15 commented Nov 29, 2021

My apologies. I didn't look that cloesly.

@jmark99
Copy link
Contributor Author

jmark99 commented Nov 29, 2021

@EdColeman I must admit I did not consider mocking. The test does seems to be a bit of an overkill for a one line bug fix, but I wanted to verify it using the actual command calls. I haven't had a lot of experience using mocking in the past so that probably played a part in me not going that route.

As for breaking up the IT, we could put in an issue if someone wanted to take on that task.

@milleruntime
Copy link
Contributor

The fix seems small enough that you didn't need both PRs but either way, I approved the other one.

Added a couple of grammatical fixes based upon @millerruntime suggestions.
@jmark99 jmark99 merged commit b94ce9a into apache:1.10 Nov 29, 2021
@jmark99 jmark99 deleted the getsplits1x branch November 29, 2021 18:46
@ctubbsii ctubbsii added this to In progress in 1.10.2 via automation Nov 30, 2021
@ctubbsii ctubbsii added this to In progress in 2.1.0 via automation Nov 30, 2021
@ctubbsii ctubbsii linked an issue Nov 30, 2021 that may be closed by this pull request
@ctubbsii ctubbsii removed this from In progress in 2.1.0 Nov 30, 2021
@ctubbsii ctubbsii moved this from In progress to Done in 1.10.2 Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
1.10.2
  
Done
Development

Successfully merging this pull request may close these issues.

Incorrect scan range in Accumulo shell getsplits command.
4 participants