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.

Sign up for GitHub

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

Jump to bottom

Removes unnecessary HBase injections #215

Merged
merged 1 commit into from
May 18, 2015
Merged

Removes unnecessary HBase injections #215

merged 1 commit into from
May 18, 2015

Conversation

benpence
Copy link
Contributor

The injection roles are satisfied by their bijection counterparts. In fact, explicitly calling the injections can cause a StackOverflowError as the injections satisfy the bijection implicit dependencies and vice versa.

Thank you @ianoc for your help.

@benpence
Removes unnecessary HBase injections
ff05c2a 
The injection roles are satisfied by their bijection counterparts. In fact,
explicitly calling the injections can cause a StackOverflowError as the
injections satisfy the bijection implicit dependencies and vice versa.

Thank you @ianoc for your help.
@MansurAshraf
Copy link
Contributor

How about adding some tests that use injections?

ianoc added a commit that referenced this pull request May 18, 2015
@ianoc
Merge pull request #215 from benpence/remove_hbase_injections
7dcf968 
Removes unnecessary HBase injections
@ianoc ianoc merged commit 7dcf968 into twitter:develop May 18, 2015
@johnynek
Copy link
Collaborator

  1. we should keep the Injections, not the Bijections (which were a hack we used before we added the Injection type).

  2. If we don't want to change the code much, just don't make the first set of Injections that Mansur added, but the last set (using ImmutableBytesWritable), should still have been fine.

@ianoc
Copy link
Collaborator

ianoc commented May 18, 2015

the last set were the ones causing the stack overflow errors ben saw

@johnynek
Copy link
Collaborator

Well, in any case, we should keep Injections here, not the Bijection with the @@ type. In fact, I think we should remove the @@ type from the library. I've never seen anyone use it and it confuses new users.

@benpence
Copy link
Contributor Author

OK I'll work to remove the bijections and rewrite the tests

benpence added a commit to benpence/bijection that referenced this pull request May 28, 2015
@benpence
Revert "Merge pull request twitter#215 from benpence/remove_hbase_inj…
467358e 
…ections"

This reverts commit 7dcf968, reversing
changes made to a428acd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@benpence @MansurAshraf @johnynek @ianoc