You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2016/08/01 23:17:40 UTC

Review Request 50680: fix replace on overflowed entry

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/
-----------------------------------------------------------

Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.


Bugs: GEODE-1718
    https://issues.apache.org/jira/browse/GEODE-1718


Repository: geode


Description
-------

New unit test demonstrated that replace would return false when done on an overflowed entry.
With the fix the test passes.
Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
If obj is null checkPdxEquals will now call basicEquals which handles testing null.


Diffs
-----

  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
  geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/50680/diff/


Testing
-------

precheckin


Thanks,

Darrel Schneider

Re: Review Request 50680: fix replace on overflowed entry

Posted by Barry Oglesby <bo...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144660
-----------------------------------------------------------


Ship it!




Ship It!

- Barry Oglesby


On Aug. 3, 2016, 6:36 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2016, 6:36 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 50680: fix replace on overflowed entry

Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/
-----------------------------------------------------------

(Updated Aug. 3, 2016, 11:36 a.m.)


Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.


Bugs: GEODE-1718
    https://issues.apache.org/jira/browse/GEODE-1718


Repository: geode


Description
-------

New unit test demonstrated that replace would return false when done on an overflowed entry.
With the fix the test passes.
Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
If obj is null checkPdxEquals will now call basicEquals which handles testing null.


Diffs (updated)
-----

  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
  geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
  geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 

Diff: https://reviews.apache.org/r/50680/diff/


Testing
-------

precheckin


Thanks,

Darrel Schneider

Re: Review Request 50680: fix replace on overflowed entry

Posted by Barry Oglesby <bo...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144405
-----------------------------------------------------------


Ship it!




Ship It!

- Barry Oglesby


On Aug. 1, 2016, 11:17 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2016, 11:17 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 50680: fix replace on overflowed entry

Posted by Darrel Schneider <ds...@pivotal.io>.

> On Aug. 3, 2016, 11:35 a.m., Darrel Schneider wrote:
> > This fix has a problem. It always reads the value from disk if it had overflowed.
> > But just a couple of lines before this basicPut has called setOldValueInEvent.
> > I think the correct fix is to use that old value from the event instead of rereading from disk/memory.

I have posted a new review that has a fix that does not result in reading the value twice


- Darrel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144640
-----------------------------------------------------------


On Aug. 3, 2016, 11:36 a.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2016, 11:36 a.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 50680: fix replace on overflowed entry

Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144640
-----------------------------------------------------------



This fix has a problem. It always reads the value from disk if it had overflowed.
But just a couple of lines before this basicPut has called setOldValueInEvent.
I think the correct fix is to use that old value from the event instead of rereading from disk/memory.

- Darrel Schneider


On Aug. 1, 2016, 4:17 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2016, 4:17 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 50680: fix replace on overflowed entry

Posted by Ken Howe <kh...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144503
-----------------------------------------------------------


Ship it!




Ship It!

- Ken Howe


On Aug. 1, 2016, 11:17 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2016, 11:17 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>

Re: Review Request 50680: fix replace on overflowed entry

Posted by Eric Shu <es...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50680/#review144407
-----------------------------------------------------------


Ship it!




Ship It!

- Eric Shu


On Aug. 1, 2016, 11:17 p.m., Darrel Schneider wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50680/
> -----------------------------------------------------------
> 
> (Updated Aug. 1, 2016, 11:17 p.m.)
> 
> 
> Review request for geode, Barry Oglesby, Eric Shu, Scott Jewell, and Ken Howe.
> 
> 
> Bugs: GEODE-1718
>     https://issues.apache.org/jira/browse/GEODE-1718
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> New unit test demonstrated that replace would return false when done on an overflowed entry.
> With the fix the test passes.
> Also added a null check in checkPdxEquals to prevent NPE if obj is ever null.
> If obj is null checkPdxEquals will now call basicEquals which handles testing null.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionEntry.java 15a5bed750b676c668ddde64291a5b315fca90fd 
>   geode-core/src/main/java/com/gemstone/gemfire/internal/cache/AbstractRegionMap.java f3cb3d654087c797de430f448348c3ba66846ac4 
>   geode-core/src/test/java/com/gemstone/gemfire/internal/cache/ReplaceWithOverflowJUnitTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50680/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Darrel Schneider
> 
>