[
https://issues.apache.org/jira/browse/BOOKKEEPER-345?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13452870#comment-13452870
]
Sijie Guo commented on BOOKKEEPER-345:
--------------------------------------
thanks Vinay for new patch. please check my comments as below.
{code}
+ void transitionToReadOnlyMode() {
+ if (readOnly.get()) {
+ return;
+ }
+ readOnly.set(true);
{code}
the code could not prevent two callers to call the function at the same time.
you need to use 'compareAndSet'.
{code}
if (null == zk.exists(this.bookieRegistrationPath + "readonly",
+ false)) {
+ // Create the readonly node
+ zk.create(this.bookieRegistrationPath + "readonly" + "/"
+ + getMyId(), new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.EPHEMERAL);
{code}
it would be better to define 'readonly' as constant string.
{code}
+ if (null == zk.exists(this.bookieRegistrationPath + "readonly",
+ false)) {
+ zk.create(this.bookieRegistrationPath + "readonly",
+ new byte[0], Ids.OPEN_ACL_UNSAFE,
CreateMode.PERSISTENT);
+ }
{code}
you should catch NodeExistsException for the creation. if others created
'readonly' znode, you could continue the logic to enter readonly mode.
{code}
public void recoveryAddEntry(ByteBuffer entry, WriteCallback cb, Object
ctx, byte[] masterKey)
throws IOException, BookieException {
- LedgerDescriptor handle = getLedgerForEntry(entry, masterKey);
- synchronized (handle) {
- addEntryInternal(handle, entry, cb, ctx);
+ try {
+ LedgerDescriptor handle = getLedgerForEntry(entry, masterKey);
+ synchronized (handle) {
+ addEntryInternal(handle, entry, cb, ctx);
+ }
+ } catch (NoWritableLedgerDirException e) {
+ transitionToReadOnlyMode();
+ throw new IOException(e.getMessage(), e);
}
}
{code}
why not 'throw new IOException(e)'? since you just use the error message from
underlying exception. same issue in addEntry.
{code}
- if (logChannel.position() + entry.remaining() + 4 > logSizeLimit) {
+ // Create new log if logSizeLimit reached or current disk is full
+ boolean createNewLog = shouldCreateNewEntryLog.get();
+ if (createNewLog
+ || (logChannel.position() + entry.remaining() + 4 >
logSizeLimit)) {
createNewLog();
+
+ // Reset the flag
+ if (createNewLog) {
+ shouldCreateNewEntryLog.set(false);
+ }
}
{code}
If it happens to following case, does it make the entry log corrupted?
the case is: there is no enough space to store an entry, but the
ledgerDirsManager doesn't find that the disk is out-of-usage. (user configure a
larger enough threshold).
It is a bit heavier here to operate the writableLedgerDirectories. why not make
writableLedgerDirectories, filledDirs as volatile, and change them when
addToFilledDirs. so the getWritableLedgerDirs would be simple.
{code}
+ public List<File> getWritableLedgerDirs()
+ throws NoWritableLedgerDirException {
List<File> dirs = writableLedgerDirectories;
+ if (dirs.isEmpty()) {
+ String errMsg = "All ledger directories are non writable";
+ NoWritableLedgerDirException e = new NoWritableLedgerDirException(
+ errMsg);
+ LOG.error(errMsg, e);
+ throw e;
+ }
+ return dirs;
+ }
{code}
{code}
+ /**
+ * The server is running as read-only mode
+ */
+ public static final int READONLY = 105;
{code}
since all the error codes are prefixed with 'E', it would be better to keep
naming consistent. And I think it would be better to add a specific exception
for writes to readonly bookie in client-side.
> 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
> Assignee: Vinay
> Fix For: 4.2.0
>
> Attachments: BOOKKEEPER-345.patch, BOOKKEEPER-345.patch,
> BOOKKEEPER-345.patch, BOOKKEEPER-345.patch, 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
For more information on JIRA, see: http://www.atlassian.com/software/jira
