[
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-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
