geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Darrel Schneider <dschnei...@pivotal.io>
Subject Re: Review Request 56506: GEODE-2398: Retry oplog channel.write on silent failures
Date Fri, 10 Feb 2017 22:02:52 GMT

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




geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java (line 5192)
<https://reviews.apache.org/r/56506/#comment236950>

    I think the code would be easier to understand if these variables were declared inside
the "if" block or "do" block that uses them instead of all at the beginning of the method.
Doing this can also make refactoring large blocks into their own method easier.
    
    My understanding is that you will not get any performance improvement over declaring all
local variables at the start of a method.
    
    I'd also encourage using "final" on local variables that that code only initializes and
never change.



geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java (line 5213)
<https://reviews.apache.org/r/56506/#comment236951>

    What we have seen so far is that when something is wrong it has always been the position
in "bb".
    This code basically says that if the result of "channel.write" is not consistent with
the "bb" position then use the "channel" position as the final authority on what is correct.
    It is possible that the "bb" position is correct according to the "channel" position.
So when you fix the "bb" position to be consistent with the channel, I think you should also
fix the result of "channel.write" (stored in "channelBytesWritten"). In the retry block you
can just update both of these. Then move the inc of "flushed" to be done after this block
(right before "while bb.hasRemaining").



geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java (line 5223)
<https://reviews.apache.org/r/56506/#comment236824>

    It would be nice to include more info in this IOException related to the inconsistency
in channleBytesWritten vs. bytes read from bb.


- Darrel Schneider


On Feb. 9, 2017, 10:11 a.m., Ken Howe wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56506/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 10:11 a.m.)
> 
> 
> Review request for geode, anilkumar gingade and Darrel Schneider.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Implemented limited retries in two forms of Oplog.flush() when channel.write() is called.
> If write() returns bytes witten less than the change in the ByteBuffer positions, then
reset
> buffer positions and re-try writing for a limited number of times. Throws
> IOException if the write doesn't succeeded after a few retries (max
> number of retries is defined by a static).
> 
> Added new unit tests.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java 0b98364b743c39e69773d586f9c793eb7de71b8d

>   geode-core/src/test/java/org/apache/geode/internal/cache/OplogFlushTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/56506/diff/
> 
> 
> Testing
> -------
> 
> Started precheckin
> 
> 
> Thanks,
> 
> Ken Howe
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message