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 for #1064. #1070

Merged
merged 7 commits into from Apr 2, 2019

Conversation

@keith-turner
Copy link
Contributor

commented Apr 1, 2019

No description provided.

@milleruntime

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

NativeMapIT passes with this fix on Fedora 28 with gcc-8.3.1-2.fc28.src.rpm and gcc-8.0.1-0.20.fc28.src.rpm

@ctubbsii

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

This native code is hard to review for somebody used to looking at OO code. The native code is in desperate need of some sort of documented developer information to explain the overall design... even a few inline comments here and there to explain how the objects are supposed to be used would be helpful (For example, what is a SubKey? Or a Field? And what is going on with "blocks" vs. "bigBlocks" vectors that has been the cause of these last two bugs?)

I'm happy if the tests pass, but I don't have a lot of confidence that it's doing the right thing in all circumstances.

@milleruntime

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

This native code is hard to review for somebody used to looking at OO code. The native code is in desperate need of some sort of documented developer information to explain the overall design... even a few inline comments here and there to explain how the objects are supposed to be used would be helpful (For example, what is a SubKey? Or a Field? And what is going on with "blocks" vs. "bigBlocks" vectors that has been the cause of these last two bugs?)

I'm happy if the tests pass, but I don't have a lot of confidence that it's doing the right thing in all circumstances.

I agree. Anything to make this code more maintainable would be very helpful.

@ctubbsii ctubbsii added v2.0.0 labels Apr 2, 2019

keith-turner added some commits Apr 2, 2019

WIP
@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@ctubbsii @milleruntime I added some comments to the code

@milleruntime

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

I think this is good for 1.9. For 2.0 can we refactor some of these variables to be more meaningful... like linkBlockAllocator instead of lba.

@keith-turner keith-turner marked this pull request as ready for review Apr 2, 2019

keith-turner added some commits Apr 2, 2019

@keith-turner keith-turner changed the title A possible fix for #1064. I am still reviewing the code. Fix for #1064. Apr 2, 2019

@keith-turner keith-turner merged commit 8928726 into apache:1.9 Apr 2, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Github just goofed up my merge for #1070. I pressed "squash and merge" and it said there was an error and presented a "try again" button. So I pressed that and it did a merge commit instead of squashing.

@keith-turner keith-turner deleted the keith-turner:accumulo-1064 branch Apr 2, 2019

@ctubbsii ctubbsii added this to Done in 1.9.3 Jun 14, 2019

@ctubbsii ctubbsii added this to Done in 2.0.0 Jun 14, 2019

@joshelser

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Github just goofed up my merge for #1070. I pressed "squash and merge" and it said there was an error and presented a "try again" button. So I pressed that and it did a merge commit instead of squashing.

Can you revert and re-apply as a squash in the future? Running into this pain now :)

@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Can you revert and re-apply as a squash in the future? Running into this pain now :)

I decided to just leave it in order to avoid as it was instead of doing a force push to fix it.

@joshelser

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

instead of doing a force push to fix it.

A revert of the previous commits and re-application of your commits as one wouldn't have required a force-push :)

@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

A revert of the previous commits and re-application of your commits as one wouldn't have required a force-push :)

Right. I was being lazy and didn't fully explain what I meant by fix it. From my perspective the problem at the time was that the merge made the history messier than it needed to be. I could not think of anything besides force push achieved my goal of making history cleaner, so I decided to just leave it.

@keith-turner

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Can you revert and re-apply as a squash in the future? Running into this pain now :)

I suppose revert and re-apply with good commit messages explaining what happened could make history cleaner. But the extra commits seems to add noise, not sure which is best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.