HADOOP-16948. Support single writer dirs. #1925
Conversation
|
hey billie, good to have you submitting code again. Looks like it doesn't merge as we've broken things already. Which abfs endpoint did you run all the existing tests against? |
There was a problem hiding this comment.
this seems to be recurrent merge pain point: too many patches adding more things to every rest call.
Proposed: how about adding a RestOperationContext struct which gets passed down, leaseId would go in there, and later other stuff (statistics, trace context, etc) ?
There was a problem hiding this comment.
PathIOException with path; make error string a const to use when matching in tests. Consider also a new LeaseRequiredException if that helps testing
...ools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsOutputStream.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
switch to SLF4J logging style; include full stack @ debug level
There was a problem hiding this comment.
should be tied in to the FileSystem instance lifecycle too: an FS instance should really have a weak ref to all leases created under it, and fs.close to stop them all
|
Thanks for the review, @steveloughran! I'll work on addressing your comments. |
|
As there are clients using ABFS accounts with HNS enabled and not enabled, we usually publish test results from both type of accounts. [Test config details; https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-azure/src/site/markdown/testing_azure.md#testing-the-azure-abfs-client] When you update the PR next, could you also please add unit tests around SelfRenewingLease and append, flush Abfsclient methods with lease valid/invalid cases. |
billierinaldi
force-pushed
the
abfs-lease-ops
branch
from
d98c26c to
d665879
Compare
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
Looking good.
- Afraid you will need to rebase...that might make those test failures go away
- there's the usual 'declare your test endpoint' policy
- and someone who understands the abfs client better than me will need to comment about the low level stuff.
Lifecycle changes seem good though.
Looking in Guava, I see a ListeningScheduledExecutorService. Do we actually have to have 1 thread per lease, or would it be better to have an executor in the ABFS client, and each lease simply schedules work in there at the given rate?
...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
Outdated
Show resolved
Hide resolved
...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
Outdated
Show resolved
Hide resolved
...ols/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/SelfRenewingLease.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Prefer you use our normal RetryPolicy if possible
|
Thanks for reviewing again, @steveloughran! I appreciate your helpful comments and will look into those. |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
just had a quick look @ this and made some more comments.
shall we try and get this in in 2020?
There was a problem hiding this comment.
how about using DurationInfo in the try with resources (logging @ debug) to track how long acquire/release took. I can imagine it can take a while to acquire. Indeed, do we have to worry about timeouts, heartbeats, etc?
There was a problem hiding this comment.
o.a.h.utils.IOUtils.close methods do this
There was a problem hiding this comment.
GenericTestUtils lets you assert something is in the error message. Its critical to rethrow (maybe wrapped) the exception if it is not the one you were expecting
There was a problem hiding this comment.
and add a message to raise if the condition is met.
note, it's ok to use AssertJ for your asserts, we are adopting it more broadly and enjoying its diagnostics.
billierinaldi
force-pushed
the
abfs-lease-ops
branch
from
bb65e71 to
43faa39
Compare
|
I have only tested with HNS so far, so I am going to try testing without HNS now. |
steveloughran
added
enhancement
fs/azure
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
looks good. some minor nits on imports &c
- need some documentation, including the whole lease mechanism (how to use...)
- I worry about raising exceptions on close(). A lot of code doesn't expect it. Is it just outputstream.close() when there is data buffered to be written? If so, that's ok
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Prefer you use HadoopExecutors here.
There was a problem hiding this comment.
HadoopExecutors.shutdown has some error handling here
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
...tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AzureBlobFileSystemStore.java
Outdated
Show resolved
Hide resolved
.../hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemLease.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
use LambdaTestUtils; return a string with that error message in the closure for it to be used in the exception. Ideally add out.toString() too. eg.
intercept(ioe, ERR_LEASE_EXPIRED, () -> {
out..write(1);
out.hsync();
return "expected exception but got " + out;
});
.../hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemLease.java
Outdated
Show resolved
Hide resolved
|
I think I have addressed the last round of review comments, including adding some documentation on single writer directories to abfs.md and adding more informative javadocs to the lease methods in AzureBlobFileSystem. Let me know if there is anything unclear or needing more explanation in the docs. I am still working on getting my environment set up to run dev-support/testrun-scripts/runtests.sh, so I don't have test results to post yet, but hopefully I will have those soon. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
this is coming along nicely
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think I'd keep that t text in the superclass text, in case a deep tree causes the nested cause not to be listed.
but: use toString() (implicitly) rather than getMessage, because some exceptions (NPW) have a null message.
There was a problem hiding this comment.
just rethrow it or wrap in an assertion error. we need that full stack trace
.../hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemLease.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
little architecture question. Would this be better in the Store than the FS? I don't know, and it is higher level than the Rest API, isn't it? Which implies this is the right place.
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
looking good. production code is done; just some minor nits about the tests being informative on future failures, which means: just rethrow exceptions when not expected, assertTrue/assertFalse to include some message about what failed. thanks
There was a problem hiding this comment.
good writeup
- is there any validation here, that if a path in the local FS is to be leased then the executor count must be >1?
- what if I'm working with >1 FS? Will this configuration be per-fs? Or does it take a list of paths which can be full URIs to paths in a store? That's what we ended up doing with s3a authoritative paths0
There was a problem hiding this comment.
is there any validation here, that if a path in the local FS is to be leased then the executor count must be >1?
Yes, in SelfRenewingLease it throws an exception if there are < 1 lease threads.
what if I'm working with >1 FS? Will this configuration be per-fs? Or does it take a list of paths which can be full URIs to paths in a store?
I believe the single writer dirs config accepts a list of full URIs -- I will double check -- but they all share the same pool of lease threads.
I am also looking into whether it makes sense to make the lease duration configurable. This would allow configuration of a finite or infinite lease duration, and in the infinite lease case we could avoid frequent calls to the Azure API to renew the lease. (For an infinite lease, if the client stopped without releasing the lease, the lease would have to be explicitly broken for a different writer to obtain a new lease on the file. It's a tradeoff, and I could imagine both finite and infinite lease options being useful.)
There was a problem hiding this comment.
Correction, single writer dirs accepts a list of paths, not URIs.
billierinaldi
force-pushed
the
abfs-lease-ops
branch
from
978cedb to
a176dc7
Compare
|
OK, I'm happy with this; test changes are in, and the next step is to merge and see what happens to people using the feature. Billie: +1 from me; if you are happy with it yourself then merge into trunk at your leisure. |
There was a problem hiding this comment.
Background threads that will renew lease every 67% of lease i.e. 10 seconds for 15 second lease and 40 seconds for 60 second lease will add extra cost to customers
There was a problem hiding this comment.
Thanks Sneha, I didn't realize the lease renewals would be charged as write ops. It might be a poor experience for a user to have unexpected charges related to this lease configuration.
There was a problem hiding this comment.
Hey Billie just wanted to add that it might not be charged as write ops but instead come under other ops or metadata ops. Either way it will be extra cost just not as high as write transaction charges.
There was a problem hiding this comment.
Please check if infinite lease is sufficient for your use case.
There was a problem hiding this comment.
I think infinite leases are sufficient for my use case. I would be okay with removing lease renewal from this patch and leaving finite leases for future work in HADOOP-17590, but I am not sure what the best way to handle the configuration properties would be. It sounds like you are proposing a boolean fs.azure.write.enforcelease that would control whether lease ops are applied for all files, and all files would have the same finite lease duration, is that right? I am wondering how to make that work together with the fs.azure.singlewriter.directories property in this patch. Would we want to specify a special set of directories that uses infinite leases? Or do we need to figure out a way to specify lease duration for each directory that supports lease ops?
There was a problem hiding this comment.
Yes, We should specify a special set of directories that uses infinite leases. By default we would keep 60 seconds as lease duration for all files.
Or do we need to figure out a way to specify lease duration for each directory that supports lease ops?
This will not scale.
There was a problem hiding this comment.
So, does the property name fs.azure.singlewriter.directories still make sense, or should it be changed to something else such as fs.azure.infinite-lease-directories?
There was a problem hiding this comment.
Agree, fs.azure.singlewriter.directories would confuse the users. We can name it something else.
There was a problem hiding this comment.
Error handling for cases when append may take more time than lease expiry needs to be added incase there is a finite lease.
There was a problem hiding this comment.
this code should not be required
There was a problem hiding this comment.
I'm not sure what you mean. Do you mean that we shouldn't have an acquireLease method in the store because leases will be acquired automatically?
There was a problem hiding this comment.
Yes if the file is being created then infinite lease can be automatically taken, for other cases yes you may need the acquire lease code till we integrate bundling of lease with append.
RenewLease code might be something you can completely get rid of
|
💔 -1 overall
This message was automatically generated. |
billierinaldi
force-pushed
the
abfs-lease-ops
branch
from
beade4c to
822615e
Compare
|
🎊 +1 overall
This message was automatically generated. |
| DefaultValue = DEFAULT_FS_AZURE_INFINITE_LEASE_DIRECTORIES) | ||
| private String azureInfiniteLeaseDirs; | ||
|
|
||
| @IntegerConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_LEASE_THREADS, |
There was a problem hiding this comment.
Do we need these? i.e. Lease threads
There was a problem hiding this comment.
I think it will still be useful to issue the acquire and release operations in a thread pool for now. Possibly this could be removed if all acquire and release operations are moved into create and flush-with-close in the future.
|
🎊 +1 overall
This message was automatically generated. |
| DefaultValue = DEFAULT_FS_AZURE_APPEND_BLOB_DIRECTORIES) | ||
| private String azureAppendBlobDirs; | ||
|
|
||
| @StringConfigurationValidatorAnnotation(ConfigurationKey = FS_AZURE_INFINITE_LEASE_KEY, |
There was a problem hiding this comment.
Is the feature for both namespace and flatnamespace enabled accounts?
There was a problem hiding this comment.
I have run the unit test with HNS and flat namespace storage accounts, so I think it will work. I have not done extensive testing with HNS disabled, however.
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
LGTM. One question about the while() loop during lease acquistion -is busy wait really the right approach here? I think you should use Thread.yield() if you aren't going to switch to any of the classic concurrency classes
| LEASE_ACQUIRE_MAX_RETRIES, LEASE_ACQUIRE_RETRY_INTERVAL, TimeUnit.SECONDS); | ||
| acquireLease(retryPolicy, 0, 0); | ||
|
|
||
| while (leaseID == null && exception == null) { |
There was a problem hiding this comment.
This looks a CPU-heavy loop. I know it makes for a more responsive app, but it's a busy wait. Any way to replace with some concurrency class?
There was a problem hiding this comment.
Good point. We will have the Future at that point, so we could wait for it to complete.
There was a problem hiding this comment.
I pushed a new change to address this. I am also looking into figuring out if I can mock an acquire lease failure to test this out a bit better.
| try { | ||
| if (RetryPolicy.RetryAction.RetryDecision.RETRY | ||
| == retryPolicy.shouldRetry(null, numRetries, 0, true).action) { | ||
| LOG.debug("Failed acquire lease on {}, retrying: {}", path, throwable); |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| } catch (Exception e) { | ||
| LOG.debug("Got exception waiting for acquire lease future. Checking if lease ID or " | ||
| + "exception have been set", e); | ||
| } |
There was a problem hiding this comment.
if this raises an exception, is there any way the while loop will exit?
There was a problem hiding this comment.
Yes, in this section it will retry if it is below the max retries and otherwise set the exception variable. So by max retries we should either have a lease ID or an exception set, and the while loop will exit. In the unit test, I mocked two failures followed by a success as well as persistent failure and verified it had the correct behavior.
|
@billierinaldi -I'm happy with this. There may be some surprises once you go live, but there's nothing obvious to me right now. +1. merge when ready either from the button or the terminal. If you plan to backport to the 3.3.x line, cherry pick in to branch-3.3 and do a new test run. thanks |
…lie Rinaldi. (cherry picked from commit c1fde4f)
* HADOOP-16948. Support single writer dirs. * HADOOP-16948. Fix findbugs and checkstyle problems. * HADOOP-16948. Fix remaining checkstyle problems. * HADOOP-16948. Add DurationInfo, retry policy for acquiring lease, and javadocs * HADOOP-16948. Convert ABFS client to use an executor for lease ops * HADOOP-16948. Fix ABFS lease test for non-HNS * HADOOP-16948. Fix checkstyle and javadoc * HADOOP-16948. Address review comments * HADOOP-16948. Use daemon threads for ABFS lease ops * HADOOP-16948. Make lease duration configurable * HADOOP-16948. Add error messages to test assertions * HADOOP-16948. Remove extra isSingleWriterKey call * HADOOP-16948. Use only infinite lease duration due to cost of renewal ops * HADOOP-16948. Remove acquire/renew/release lease methods * HADOOP-16948. Rename single writer dirs to infinite lease dirs * HADOOP-16948. Fix checkstyle * HADOOP-16948. Wait for acquire lease future * HADOOP-16948. Add unit test for acquire lease failure
). Contributed by Billie Rinaldi. (cherry picked from commit c1fde4f) Change-Id: I10bd8ff89649ced3498693c1386da29dc3cc8f40
5 participants
So far, I have tested this patch by opening an output stream, writing, and syncing (via hflush or hsync). Then I broke the lease on the file and tried to write and sync again to the original output stream, obtaining an expected exception. After that, I closed the file and verified that the file contained the data from the first write but not the second write.