hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Steve Loughran (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-14520) WASB: Block compaction for Azure Block Blobs
Date Wed, 16 Aug 2017 18:11:00 GMT

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

Steve Loughran commented on HADOOP-14520:
-----------------------------------------

This is quite a big patch; I've had to learn my way round bits of code while reviewing it.
For that reason alone, I'm not knowledgeable enough to be the sole reviewer. What I have done
is gone through the code & tried to understand what it's working with, comments below.
The bad news is, because it's code I'm not familiar with, (a) my comments go further than
just this patch and (b) I may be utterly wrong. Bear that in mind.

Here's my first review, though its not detailed enough.

h3. {{AzureNativeFileSystemStore}}


* It looks like {{AzureNativeFileSystemStore.getDirectorySet()}} doesn't trim whitespace from
paths. Created HADOOP-14778 to deal with it separately. 


h3. {{BlockBlobAppendStream}}

* if you are changing precondition check, I'd recommend StringUtils.isEmpty() for 

{code}
Preconditions.checkArgument(StringUtils.isNotEmpty(aKey));
Preconditions.checkArgument(bufferSize >= 0);
{code}

* If fields aren't updated after the constructor, best to set to {{final}} (example, {{compactionEnabled}}
?).
* How long is {{downloadBlockList}} going to take in that constructor? More specifically:
if compaction is disabled, can that step be skipped? 
* If the stream needs a byte buffer, best to use {{ElasticByteBufferPool}} as a pool of buffers.


h3. Exception handling, wrapping, rethrowing

* Use {{StorageErrorCodeStrings}} as the source of string constants to check for in exception
error codes.
* Rather than {{throw IOException(e)}}, I'd prefer more specific (existing ones). That's {{PathIOException}}
and subclasses, {{AzureException(e)}}, and the java.io/java.nio ones. Whichever is closest
to what's actually gone wrong. IOEs are too generic to use in try/catch.
* When wrapping a StorageException with another IOE, always include the toString value of
the wrapped exception.
That way, the log message of the top level log retains the underlying problem.

Example from {{UploadBlockCommand}}:

{code}
 throw new IOException("Encountered Exception while committing append blocks", ex);
{code} 

{code}
 throw new IOException("Encountered Exception while committing append blocks: " + ex, ex);
{code} 

* {{BlockBlobAppendStream.WriteRequest}} retry logic will retry even on RuntimeExceptions
like IllegalArgumentException. Ideally they should be split into recoverable vs non-recoverable
ops via a {{RetryPolicy}}. Is this an issue to address here though?

Overall, with the new operatins doing retries, this may be time to embrace rety policies.
Or at least create a JIRA entry on doing so.


h3. Concurrency


# I know {{java.io.OutputStream}} is marked as single-thread only, but I know of code (hello
HBase!) which means that you must make some of the calls thread safe. HADOOP-11708/HADOOP-11710
covers this issue in CryptoOutputStream. At the very least, {{flush()}} must be synchronous
with itself, close() & maybe write()
# I'm unsure about {{BlockBlobAppendStream.close()}} waiting for up to 15 minutes for things
to complete, but looking @ other blobstore clients, I can see that they are implicitly waiting
without any timeout at all. And it's in the existing codebase. But: why was the time limit
changed from 10 min to 15? Was this based on test failures? If so, where is the guarantee
that a 15 minute wait is always sufficient.
# Looking at {{BlockBlobAppendStream}} thread pooling, I think having a thread pool per output
stream is expensive, especially as it has a minimum size of 4; it will ramp up fast. A pool
of min=1 max=4 might be less expensive. But really, the stream should be thinking about sharing
a pool common to the FS, relying on callbacks to notify it of completion rather than just
awaiting pool completion and a shared writeable field.
# I think the access/use of {{lastException}} needs to be made stronger than just a {{volatile}},
as it means that code of the form {{if (lastException!=null) throw lastException}} isn't thread
safe. I know, it's not that harmful provided lastException is never set to null, but I'd still
like some isolated get/getAndSet/maybeThrow operations.
# Similarly, is {{lastException}} the best way to propagate failure, as it means that teardown
failures are going to get reported ahead of earlier ones during the write itself. 

Overall, I propose using Callable<> over Runnable. Allows you to throw exceptions &
return things, caller gets to pick them up when it chooses to.


h3. code style

Checkstyle has a lot of complaints (which will need a resubmit to show). 

* Can you do a patch without all the whitespace stripping? It makes the patch too big &
very brittle to cherrypick. I know the spaces are wrong, but trying to strip them in a patch
creates needless patch conflict. when the patch goes in we'll strip off whitespace on new/changed
lines. so it won't get any worse.
* Do try to get those line lengths under 80, including in comments. I know it seems out of
date, and I'd prefer a higher number, but current consensus is 80 except when it's really
hard to do. Supporting side-by-side diff comparison is a reason.
* Don't use {{import java.util.*;}} for imports —it's OK for static member import, but not
for whole packages. Makes for brittleness across versions, as when a package adds a competing
class, things stop building. I'm assuming this was the IDE being helpful here. If it has autoconvert
to * on, turn it off, along with "delete trailing whitespace".

* and add trailing "." on the first sentence of javadocs, as javadoc complains in their absence.


h3. Testing/ {{TestNativeAzureFileSystemBlockCompaction}}

# I should warn in HADOOP-14553 I'm trying to support parallel test runs in the hadoop-azure
module, if it goes in first then this patch will need modification to parallelise, at least
if a non-mock test is used.
* why don't {{verifyFileData}} and {{verifyAppend}} throw up any exceptions raised?
# Use try-with-resources & you can get streams closed automatically


h3. index.md

* use `` around method names. 
* Need to emphasize cost compaction: a download of segments and uplod of a compacted replacement.
Only for use when the Azure storage is on the same site/network, cost of bandwidth & time.
Do provide details on failure guarantees too, like "doesn't ever lose data".

h3. Other points

It would seem good to have some FS statistics tracking compaction, e.g: #of compaction events,
bytes downloaded from compaction, bytes uploaded. These can then be used in assertions in
the tests, but, most importantly, can be looked at in production to see why things are slow
and/or whether compaction is working.


> WASB: Block compaction for Azure Block Blobs
> --------------------------------------------
>
>                 Key: HADOOP-14520
>                 URL: https://issues.apache.org/jira/browse/HADOOP-14520
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: fs/azure
>    Affects Versions: 3.0.0-alpha3
>            Reporter: Georgi Chalakov
>            Assignee: Georgi Chalakov
>         Attachments: HADOOP-14520-05.patch
>
>
> Block Compaction for WASB allows uploading new blocks for every hflush/hsync call. When
the number of blocks is above 32000, next hflush/hsync triggers the block compaction process.
Block compaction replaces a sequence of blocks with one block. From all the sequences with
total length less than 4M, compaction chooses the longest one. It is a greedy algorithm that
preserve all potential candidates for the next round. Block Compaction for WASB increases
data durability and allows using block blobs instead of page blobs. By default, block compaction
is disabled. Similar to the configuration for page blobs, the client needs to specify HDFS
folders where block compaction over block blobs is enabled. 
> Results for HADOOP-14520-05.patch
> tested endpoint: fs.azure.account.key.hdfs4.blob.core.windows.net
> Tests run: 707, Failures: 0, Errors: 0, Skipped: 119



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message