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

AVRO-1882: ConcurrentHashMap with non-string keys fails in Java 1.8 #113

Closed
wants to merge 4 commits into from

Conversation

sachingsachin
Copy link

@sachingsachin sachingsachin commented Jul 25, 2016

The new MapEntry class removes any dependence on the Map.Entry internal fields which are liable to change between Java versions or between different implementations of Map interface.

Also updated maven-shade-plugin as per this thread

@rdblue
Copy link
Contributor

rdblue commented Jul 25, 2016

@sachingsachin, this looks fine to me, but I think the maven-shade-plugin change should be done in a separate issue and pull request. Could you open a new issue and separate that change out? Thanks!

@sachingsachin
Copy link
Author

sachingsachin commented Jul 25, 2016

@rdblue Removed the shade plugin upgrade and put it in another pull request in AVRO-1889

@@ -17,15 +17,20 @@
*/
package org.apache.avro.reflect;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you revert moving these static imports? If there's no need to reorder then it is easier for maintenance if we make as few changes as possible.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@rdblue
Copy link
Contributor

rdblue commented Aug 28, 2016

@sachingsachin, I found a couple of other minor things to fix. Once those are done I'll merge this. Thanks for taking the time to fix it!

@sachingsachin
Copy link
Author

Thanks @rdblue for looking into this.

@sachingsachin
Copy link
Author

@rdblue , its been a month since we collaborated on this.
I have fixed the requested comments.
If you need any help from my side, then let me know.

@rdblue
Copy link
Contributor

rdblue commented Oct 30, 2016

@sachingsachin, I've committed this. Please close the PR. Thanks for fixing this!

@sachin-goyal
Copy link

Ryan Blue committed this as 420824c

@rdblue
Copy link
Contributor

rdblue commented Nov 5, 2016

@sachingsachin, can you close this PR please? Thanks!

iemejia pushed a commit that referenced this pull request Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants