You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jianxia Chen <jc...@pivotal.io> on 2016/02/25 20:32:03 UTC
Review Request 44035: PdxSerializationException Error when putall an
object from native client withSecurity enabled
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/
-----------------------------------------------------------
Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
Repository: geode
Description
-------
Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java b6d8c49
Diff: https://reviews.apache.org/r/44035/diff/
Testing
-------
Thanks,
Jianxia Chen
Re: Review Request 44035: PdxSerializationException Error when putall
an object from native client withSecurity enabled
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review120748
-----------------------------------------------------------
I seem to recall something about checking for Tokens, too. Am I making that up or is it not a concern anymore?
- Bruce Schuchardt
On Feb. 25, 2016, 7:32 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:32 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java b6d8c49
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: PdxSerializationException Error when putall
an object from native client withSecurity enabled
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review120745
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java (line 153)
<https://reviews.apache.org/r/44035/#comment182219>
Can this class be moved to an internal package. We really don't want customers to see it in the external javadocs and now that you have made it public it will show up. Since it is static it should be easy to move out of PutAllOperationContext into an internal package.
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java (line 227)
<https://reviews.apache.org/r/44035/#comment182214>
did you need to make EntrySet public for this fix?
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java (line 236)
<https://reviews.apache.org/r/44035/#comment182216>
did you need to make EntryIterator public for this fix?
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java (line 10176)
<https://reviews.apache.org/r/44035/#comment182213>
I think this code would be a little cleaner if you added a local private method like this:
private boolean isEntryValueNull(Map.Entry mapEntry) {
if (mapEntry instanceof ExportableEntry) {
return ((ExportableEntry)mapEntry).isValueNull();
} else {
return mapEntry.getValue() == null;
}
}
Then you can just call this method in the "if" and don't need to duplicate the throw code.
- Darrel Schneider
On Feb. 25, 2016, 11:32 a.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 11:32 a.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/LocalRegion.java b6d8c49
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review121534
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On March 1, 2016, 12:01 a.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated March 1, 2016, 12:01 a.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
> geode-core/src/test/java/com/gemstone/gemfire/security/ClientAuthorizationDUnitTest.java 0e46da5
> geode-core/src/test/java/com/gemstone/gemfire/security/SecurityTestUtil.java ad9b3e1
> geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/excludedClasses.txt 2097878
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review121377
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Feb. 29, 2016, 4:01 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 4:01 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
> geode-core/src/test/java/com/gemstone/gemfire/security/ClientAuthorizationDUnitTest.java 0e46da5
> geode-core/src/test/java/com/gemstone/gemfire/security/SecurityTestUtil.java ad9b3e1
> geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/excludedClasses.txt 2097878
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Jianxia Chen <jc...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/
-----------------------------------------------------------
(Updated March 1, 2016, 12:01 a.m.)
Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
Added a dunit test
Ran precheckin
Repository: geode
Description
-------
Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
geode-core/src/test/java/com/gemstone/gemfire/security/ClientAuthorizationDUnitTest.java 0e46da5
geode-core/src/test/java/com/gemstone/gemfire/security/SecurityTestUtil.java ad9b3e1
geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/excludedClasses.txt 2097878
Diff: https://reviews.apache.org/r/44035/diff/
Testing
-------
Thanks,
Jianxia Chen
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review121014
-----------------------------------------------------------
add a unit test
- Darrel Schneider
On Feb. 26, 2016, 3:27 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2016, 3:27 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/#review121013
-----------------------------------------------------------
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java (line 38)
<https://reviews.apache.org/r/44035/#comment182601>
Add a javadoc to getInternalMap saying that it should only be called by internal code that wants to bypass the exportValue method.
- Darrel Schneider
On Feb. 26, 2016, 3:27 p.m., Jianxia Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44035/
> -----------------------------------------------------------
>
> (Updated Feb. 26, 2016, 3:27 p.m.)
>
>
> Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
> geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
>
> Diff: https://reviews.apache.org/r/44035/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jianxia Chen
>
>
Re: Review Request 44035: GEODE-1014: PdxSerializationException Error
when putall an object from native client withSecurity enabled
Posted by Jianxia Chen <jc...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44035/
-----------------------------------------------------------
(Updated Feb. 26, 2016, 11:27 p.m.)
Review request for geode, Bruce Schuchardt, Darrel Schneider, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
Move UpdateOnlyMap to to an internal class, a separate file
Unwrap UpdateOnlyMap for PutAll, so that instead of UpdateOnlyMap, only the internal map will be used. Thus avoid unnecessary deserialization.
Summary (updated)
-----------------
GEODE-1014: PdxSerializationException Error when putall an object from native client withSecurity enabled
Repository: geode
Description
-------
Avoid calling PutAllOperationContext.UpdateOnlyMap.exportValue when the map entry is ExportableEntry. Because calling exportValue will result in unnecessary deserialization in this case. For this case, LocalRegion.verifyPutAllMap only need check whether the key or value is null. No deserialization is required for PutAll map verification.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/PutAllOperationContext.java b05216b
geode-core/src/main/java/com/gemstone/gemfire/cache/operations/internal/UpdateOnlyMap.java PRE-CREATION
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll.java 5d2f5ca
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll70.java 7507299
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/tier/sockets/command/PutAll80.java 6a2b072
Diff: https://reviews.apache.org/r/44035/diff/
Testing
-------
Thanks,
Jianxia Chen