[jira] [Commented] (BOOKKEEPER-345) Detect IOExceptions on entrylogger and bookie should consider next ledger dir(if any)

["Sijie Guo (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/BOOKKEEPER-345?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13431723#comment-13431723 ] 

Sijie Guo commented on BOOKKEEPER-345:
--------------------------------------

thanks Vinay, the main flow of the patch looks good. please refer my comments as below:


{code}
-    public EntryLogger(ServerConfiguration conf) throws IOException {
-        this.dirs = Bookie.getCurrentDirectories(conf.getLedgerDirs());
+    public EntryLogger(ServerConfiguration conf, Bookie bookie) throws IOException {
+        this.bookie = bookie;

-        ledgerStorage = new InterleavedLedgerStorage(conf, activeLedgerManager);
+        ledgerStorage = new InterleavedLedgerStorage(conf, activeLedgerManager, this);
         handles = new HandleFactoryImpl(ledgerStorage);
         // instantiate the journal
-        journal = new Journal(conf);
+        journal = new Journal(conf, this);
{code}

we had removed bookie reference from EntryLogger, InterleavedLedgerStorage, Journal in previous jiras, to make the dependencies clear. now you added it back gain :(
I think what you want is a place to manage all the directories for a bookie. So how about creating a separated class, like 'DirectoriesManager' (may be you would have a better name for it), which takes the responsibility of managing all directories.(NoWritableLedgerDirException would be an exception of it) And the directories manager instance could be passed for EntryLogger, LedgerStorage to use. Does it make clearer?

{code}
public class DiskChecker
{code}

It is not a good idea to let is be a static utility. You could remove static from all those methods. For a directories manager, it construct a DiskChecker instance, and read a threshold from configuration. Then the directories manager use it to check disks when exception happened. BTW, the threshold would be a percentage number not a size number.

{code}
-        void shutdown() throws InterruptedException {
+        synchronized void shutdown() {
{code}

why changing it to synchronized?

{code}
+        this.readOnlyModeEnabled = conf
+                .getBoolean("readOnlyModeEnabled", false);
{code}

It would be better to add this setting in ServerConfiguration and also put it in conf/bk_server.conf.

{code}
+    public synchronized void convertToReadOnlyMode() {
{code}

I don't think 'convert' is a better verbose. how about 'turnIntoReadOnlyMode'? I am not very sure about it, because I am not a native English speaker :-(

{code}
+    // Triggering the Bookie shutdown in separate thread.
+    // because that will shutdown sync thread also.
+    private void triggerBookieShutdown(final int exitCode) {
+        Thread shutdownThread = new Thread() {
+            public void run() {
+                if (!Bookie.this.shuttingdown) {
+                    Bookie.this.shutdown(exitCode);
+                }
+            }
+        };
+        shutdownThread.start();
+        try {
+            shutdownThread.join();
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            LOG.debug("InterruptedException while waiting for shutdown. Not a problem!!");
+        }
+    }
{code}

you don't need to check '!Bookie.this.shuttingdown'. since shutdown is synchronized.

{code}
+        // During disk full, some of the contents flush may fail.
+        // Exception also will not be thrown.
+        if (bc.position() != this.position) {
+            throw new IOException(
+                    "Error during flushing. All contents not flushed");
+        }
{code}

why need it? I remembered FileChannel would throw IOException if disk is full. please correct me if I am wrong.

{code}
+        if (!flushing) {
+            entriesDuplicateList.add(entry.duplicate());
+        }
+
{code}

I think it would buffer too much entries, if the flushing interval is long. But I don't have a better idea now. Need to see others' comments.



                
> Detect IOExceptions on entrylogger and bookie should consider next ledger dir(if any)
> -------------------------------------------------------------------------------------
>
>                 Key: BOOKKEEPER-345
>                 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-345
>             Project: Bookkeeper
>          Issue Type: Sub-task
>          Components: bookkeeper-server
>    Affects Versions: 4.1.0
>            Reporter: Rakesh R
>         Attachments: BOOKKEEPER-345.patch
>
>
> This jira to detect IOExceptions in "EntryLogger", and will iterate over all the configured ledger(s) on IOException. Finally if no writable dirs available, will move bookie to r-o mode(if this mode is enabled). 
> By default(r-o mode will be disabled) the bookie will shutdown if no writable disk available.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira