Mailing-List: contact dev-help@geode.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@geode.apache.org
Content-Type: multipart/alternative;
 boundary="===============0858583155859497549=="
MIME-Version: 1.0
Subject: Re: Review Request 56506: GEODE-2398: Retry oplog channel.write on
 silent failures
From: Darrel Schneider <dschneider@pivotal.io>
To: anilkumar gingade <agingade@pivotal.io>,
 Darrel Schneider <dschneider@pivotal.io>
Cc: geode <dev@geode.apache.org>, Ken Howe <khowe@pivotal.io>
Date: Fri, 10 Feb 2017 22:02:52 -0000
Message-ID: <20170210220252.31774.27925@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Darrel Schneider <noreply@reviews.apache.org>
References: <20170209181144.31773.96250@reviews.apache.org>
In-Reply-To: <20170209181144.31773.96250@reviews.apache.org>
Reply-To: Darrel Schneider <dschneider@pivotal.io>
archived-at: Fri, 10 Feb 2017 22:02:55 -0000

--===============0858583155859497549==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
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
> 
>


--===============0858583155859497549==--