errose28 commented on code in PR #4531:
URL: https://github.com/apache/ozone/pull/4531#discussion_r1167080975
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java:
##########
@@ -789,6 +789,36 @@ public OmKeyInfo copyObject() {
return builder.build();
}
+ /**
+ * Return a new copy of the object.
+ */
+ @Override
+ public Object clone() throws CloneNotSupportedException {
+ OmKeyInfo omKeyInfo = (OmKeyInfo) super.clone();
+
+ omKeyInfo.metadata = new HashMap<>();
+ omKeyInfo.keyLocationVersions = new ArrayList<>();
+ omKeyInfo.acls = new ArrayList<>();
+
+ keyLocationVersions.stream().filter(keyLocationVersion ->
+ keyLocationVersion != null).forEach(keyLocationVersion ->
+ omKeyInfo.keyLocationVersions.add(
+ new OmKeyLocationInfoGroup(keyLocationVersion.getVersion(),
+ keyLocationVersion.getLocationList(),
Review Comment:
> `* Use this expensive method only when absolutely needed!`
There is a risk that fixing this issue with deep copy causes a performance
regression in list keys. Since this is a pretty critical bug I think the first
version of the fix should do a shallow copy. A later implementation can decide
to either leave the fix this way, use deep copy (with perf benchmarks to
support no regressions), or fix the method to not require modifying the key
info.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java:
##########
@@ -789,6 +789,36 @@ public OmKeyInfo copyObject() {
return builder.build();
}
+ /**
+ * Return a new copy of the object.
+ */
+ @Override
+ public Object clone() throws CloneNotSupportedException {
+ OmKeyInfo omKeyInfo = (OmKeyInfo) super.clone();
+
+ omKeyInfo.metadata = new HashMap<>();
+ omKeyInfo.keyLocationVersions = new ArrayList<>();
+ omKeyInfo.acls = new ArrayList<>();
+
+ keyLocationVersions.stream().filter(keyLocationVersion ->
+ keyLocationVersion != null).forEach(keyLocationVersion ->
+ omKeyInfo.keyLocationVersions.add(
+ new OmKeyLocationInfoGroup(keyLocationVersion.getVersion(),
+ keyLocationVersion.getLocationList(),
Review Comment:
> `* Use this expensive method only when absolutely needed!`
There is a risk that fixing this issue with deep copy causes a performance
regression in list keys. Since this is a pretty critical bug I think the first
version of the fix should do a shallow copy. A later implementation can decide
to either leave the fix this way, use deep copy (with perf benchmarks to
support no regressions), or fix the method to not require modifying the key
info.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
