You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2024/09/06 14:50:40 UTC
[PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
cshannon opened a new pull request, #4859:
URL: https://github.com/apache/accumulo/pull/4859
Configure Gson to no longer use JDK sun.misc.Unsafe as that can cause some strange behavior and Unsafe could change in future JDKs. This required making sure any serialized class has a default constructor and removing the final keyword from any member variables.
Issue #4837
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747469146
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/Bulk.java:
##########
@@ -64,8 +67,11 @@ public Files getFiles() {
*/
public static class Tablet {
- private final byte[] endRow;
- private final byte[] prevEndRow;
+ private byte[] endRow;
Review Comment:
I am not sure but if the final keyword causes issues without using Unsafe, although it would still maybe cause weird behavior so still not a good idea since using reflection.
However, the thing that does cause problems is not having a default constructor. Adding in a default constructor means we can't use final so it has to be removed either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747358265
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/Bulk.java:
##########
@@ -64,8 +67,11 @@ public Files getFiles() {
*/
public static class Tablet {
- private final byte[] endRow;
- private final byte[] prevEndRow;
+ private byte[] endRow;
Review Comment:
If I were to change this to final wouid I get a runtime exception now when gson tries to deserialize it?
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServiceStatusCmd.java:
##########
@@ -152,7 +151,7 @@ private StatusSummary getServerHostStatus(final ZooReader zooReader, String base
} else {
ServiceLockData.ServiceDescriptors sld =
- GSON.get().fromJson(nodeData.getData(), ServiceLockData.ServiceDescriptors.class);
+ ServiceLockData.parseServiceDescriptors(nodeData.getData());
Review Comment:
Nice to push the serialization into the ServiceLockData class here
##########
server/base/src/main/java/org/apache/accumulo/server/util/serviceStatus/ServiceStatusReport.java:
##########
@@ -38,18 +39,21 @@ public class ServiceStatusReport {
private static final Logger LOG = LoggerFactory.getLogger(ServiceStatusReport.class);
- private static final Gson gson = new Gson();
Review Comment:
Did you search the code for uses of gson that did not use the singleton? Or did you just happen to find this?
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java:
##########
@@ -215,7 +222,37 @@ public static Optional<ServiceLockData> parse(byte[] lockData) {
}
String data = new String(lockData, UTF_8);
return data.isBlank() ? Optional.empty()
- : Optional.of(new ServiceLockData(GSON.get().fromJson(data, ServiceDescriptors.class)));
+ : Optional.of(new ServiceLockData(parseServiceDescriptors(data)));
+ }
+
+ public static ServiceDescriptors parseServiceDescriptors(String data) {
+ return deserialize(GSON.get().fromJson(data, ServiceDescriptorsGson.class));
+ }
+
+ private static ServiceDescriptors deserialize(ServiceDescriptorsGson json) {
+ return new ServiceDescriptors(json.descriptors.stream()
+ .map(s -> new ServiceDescriptor(s.uuid, s.service, s.address, s.group))
+ .collect(Collectors.toSet()));
}
+ private static class ServiceDescriptorGson {
Review Comment:
Was this added so that the ServiceDescriptor obj could continue to have final fields?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747463148
##########
core/src/main/java/org/apache/accumulo/core/lock/ServiceLockData.java:
##########
@@ -215,7 +222,37 @@ public static Optional<ServiceLockData> parse(byte[] lockData) {
}
String data = new String(lockData, UTF_8);
return data.isBlank() ? Optional.empty()
- : Optional.of(new ServiceLockData(GSON.get().fromJson(data, ServiceDescriptors.class)));
+ : Optional.of(new ServiceLockData(parseServiceDescriptors(data)));
+ }
+
+ public static ServiceDescriptors parseServiceDescriptors(String data) {
+ return deserialize(GSON.get().fromJson(data, ServiceDescriptorsGson.class));
+ }
+
+ private static ServiceDescriptors deserialize(ServiceDescriptorsGson json) {
+ return new ServiceDescriptors(json.descriptors.stream()
+ .map(s -> new ServiceDescriptor(s.uuid, s.service, s.address, s.group))
+ .collect(Collectors.toSet()));
}
+ private static class ServiceDescriptorGson {
Review Comment:
Yeah it was less refactoring to just copy to another object, like we have done in several other places. We could refactor ServiceDescriptor but then other parts of the code need updating
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747469146
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/Bulk.java:
##########
@@ -64,8 +67,11 @@ public Files getFiles() {
*/
public static class Tablet {
- private final byte[] endRow;
- private final byte[] prevEndRow;
+ private byte[] endRow;
Review Comment:
I am not sure but the `final` keyword causes issues without using Unsafe, although it would still possibly cause weird behavior so I don't think it's a good idea either way because of the use of reflection to access it and set it.
However, the thing that does cause problems is not having a default constructor. Adding in a default constructor means we can't use final so it has to be removed either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747469146
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/Bulk.java:
##########
@@ -64,8 +67,11 @@ public Files getFiles() {
*/
public static class Tablet {
- private final byte[] endRow;
- private final byte[] prevEndRow;
+ private byte[] endRow;
Review Comment:
I am not sure but the `final` keyword causes issues without using Unsafe, although it would still maybe cause weird behavior so still not a good idea since using reflection.
However, the thing that does cause problems is not having a default constructor. Adding in a default constructor means we can't use final so it has to be removed either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#issuecomment-2334491008
I have changes ready for main in a branch: https://github.com/cshannon/accumulo/commit/6ff374a9876446234fe29565de7e2f5a68dbe054
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#issuecomment-2334855534
The only failures were from missing updating the ExternalCompactionFinalState$Extent class which i fixed in https://github.com/apache/accumulo/pull/4859/commits/8f9050f945fa54ed1b4a49f2a9065ce579ae7829 so I think this should be good to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #4859:
URL: https://github.com/apache/accumulo/pull/4859
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#issuecomment-2334243560
Because most of the Gson uses in the code are using the [shared](https://github.com/apache/accumulo/blob/ab996d0d26df4b2ed2c1874289d5b785ecbe2686/core/src/main/java/org/apache/accumulo/core/util/LazySingletons.java#L39) singleton, most of the work was fixing all the errors that popped up with classes that needed to be fixed to work without using the Unsafe methods (ie a default constructor added). I think I fixed everything but I'm going to run a full IT before merging this.
I also need to take a look at main and see if this will merge cleanly. I am hoping this can be merged without too many issues and then we can just fix the additional spots in a follow on PR in main that were added by elasticity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747464085
##########
server/base/src/main/java/org/apache/accumulo/server/util/serviceStatus/ServiceStatusReport.java:
##########
@@ -38,18 +39,21 @@ public class ServiceStatusReport {
private static final Logger LOG = LoggerFactory.getLogger(ServiceStatusReport.class);
- private static final Gson gson = new Gson();
Review Comment:
I searched for all uses of gson to update and I noticed this so I switched to the singleton
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Disable Gson JDK sun.misc.Unsafe usage [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4859:
URL: https://github.com/apache/accumulo/pull/4859#discussion_r1747706463
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/bulk/Bulk.java:
##########
@@ -64,8 +67,11 @@ public Files getFiles() {
*/
public static class Tablet {
- private final byte[] endRow;
- private final byte[] prevEndRow;
+ private byte[] endRow;
Review Comment:
> Adding in a default constructor means we can't use final so it has to be removed either way.
Nice, I didn't think about the default constructor so that would cause a compile time error if someone added final.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org