[FLINK-6008][blob] collection of BlobServer improvements #4146

NicoK · 2017-06-20T09:41:45Z

This PR is a light-weight version of #3512 that only includes the improvements and can serve as a base for FLIP-19. It improves the following things around the BlobServer/BlobCache:

replace config options in config.md with non-deprecated ones, e.g. high-availability.cluster-id and high-availability.storageDir
do not fail the BlobServer when a delete operation fails
add more unit tests
general code style and docs improvements, like using Preconditions.checkArgument

NicoK · 2017-06-21T12:10:34Z

please note that there are four unrelated test failures:

2x Kafka010ITCase
2x YARNSessionCapacitySchedulerITCase

pnowojski · 2017-06-28T14:57:39Z

flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobClient.java

-		if (key == null) {
-			throw new IllegalArgumentException("BLOB key must not be null");
-		}
+		checkArgument(key != null, "BLOB key must not be null.");


requireNonNull?

unfortunately, checkNotNull throws a NullPointerException instead and I did not want to change that here

Yes, but it would be cleaner and less surprising for future users. However I have no strong feelings about it.

pnowojski · 2017-06-28T15:03:11Z

flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServerConnection.java

-					// we should make the local and remote file deletion atomic, otherwise we might risk not
-					// removing the remote file in case of a concurrent put operation
-					if (blobFile.exists() && !blobFile.delete()) {
-						throw new IOException("Cannot delete BLOB file " + blobFile.getAbsolutePath());


Don't you want to add a test coverage for not throwing if delete fails?

the (included) changes in BlobServerDeleteTest should cover this

Ok, sorry, I didn't find it first time I was looking for it.

pnowojski · 2017-06-28T15:04:33Z

flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobUtils.java

@@ -227,7 +227,7 @@ static File getStorageLocation(File storageDir, JobID jobID, String key) {
 	private static File getJobDirectory(File storageDir, JobID jobID) {
 		final File jobDirectory = new File(storageDir, JOB_DIR_PREFIX + jobID.toString());

-		if (!jobDirectory.exists() && !jobDirectory.mkdirs()) {


Why did you change the order of those operations? Is that fix for something? If so could you add explanation to the commit message what's going on here?

the new way is thread-safe - imagine a directory being concurrently created after another thread has failed the exists() part - not sure we actually fix a bug nowadays since the use may be guarded...so this is probably more of a "just-in-case" thing or if usage patterns change

Ok, please add comment or commit message info about this.

pnowojski · 2017-06-28T15:11:26Z

flink-runtime/src/test/java/org/apache/flink/runtime/blob/BlobServerPutTest.java

+
+		@Override
+		public void go() throws Exception {
+			server.getStorageLocation(key);


Shouldn't you call this at least couple/couple of dozens/couple of hundred times? Otherwise won't this complete before next thread starts up?

Actually, the job directory is only created once so it doesn't help adding more calls. What could help make this happen more often, is to add more threads to BlobServerPutTest#testServerContentAddressableGetStorageLocationConcurrent() otherwise the failure only happens once in a while if the implementation is not thread-safe

So maybe you could call in a loop multiple times gets and deletes (if there is such operation) interleaved? Otherwise I don't see a real value of those tests and I would prefer to drop them (so that we don't have to maintain tests that do not check for anything).

Unfortunately, concurrency with delete operations does not work either if not guarded - the directory may not exist anymore between jobDirectory.mkdirs() and jobDirectory.exists(). I was able to reproduce the error with the existing test though - if you want to try it, just change the order of these two commands back - the test will not hit every time, but some times.

pnowojski · 2017-06-28T15:15:17Z

flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java

 			}

+


remove extra line

…ted ones

also extend the delete tests and remove one code duplication

also add according unit tests

tillrohrmann

Thanks for your contribution @NicoK. Changes look good to me. I had two minor comments. After addressing them, we can merge this PR.

tillrohrmann · 2017-07-01T17:12:05Z

flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServerConnection.java

@@ -540,7 +526,7 @@ else if (type == NAME_ADDRESSABLE) {
 					// we should make the local and remote file deletion atomic, otherwise we might risk not
 					// removing the remote file in case of a concurrent put operation
 					if (blobFile.exists() && !blobFile.delete()) {


Shouldn't the order of the operations be changed like in the other cases?

you're right, this could be improved as well - the next PR in this series is removing this code though

tillrohrmann · 2017-07-01T17:13:53Z

flink-runtime/src/test/java/org/apache/flink/runtime/blob/BlobServerDeleteTest.java

+		catch (Exception e) {
+			e.printStackTrace();
+			fail(e.getMessage());
+		}


Can we remove this and simply let the exception be thrown? I think catching an exception, printing the stack trace and then failing with the exception message is an anti-pattern.

Absolutely right - I was told this was an issue some versions ago with JUnit or so... was just copying the code from the other tests in this class. Let me create a cleanup-PR on top of the newest PR in this series, i.e. #4238

zentol · 2017-07-04T11:05:29Z

merging,

NicoK mentioned this pull request Jun 20, 2017

[FLINK-6008] collection of BlobServer improvements #3512

Closed

NicoK mentioned this pull request Jun 21, 2017

[FLINK-7052][blob] remove (unused) NAME_ADDRESSABLE mode #4158

Closed

NicoK changed the title ~~[FLINK-6008] collection of BlobServer improvements~~ [FLINK-6008][blob] collection of BlobServer improvements Jun 26, 2017

pnowojski reviewed Jun 28, 2017

View reviewed changes

Nico Kruber added 11 commits June 30, 2017 16:01

[FLINK-6008][docs] update some config options to the new, non-depreca…

dda1a12

…ted ones

[FLINK-6008][docs] minor improvements in the BlobService docs

d54a316

[FLINK-6008] use Preconditions.checkArgument in BlobClient

e12c234

[FLINK-6008] refactor BlobCache#getURL() for cleaner code

b215515

[FLINK-6008] do not fail the BlobServer if delete fails

bbcde52

also extend the delete tests and remove one code duplication

[FLINK-6008] fix concurrent job directory creation

24060e0

also add according unit tests

[FLINK-6008] do not guard a delete() call with a check for existence

2e0d16a

[FLINK-6008] some comments about BlobLibraryCacheManager cleanup

7ba911d

[hotfix] minor typos

d3f50d5

[FLINK-6008] further cleanup tests for BlobLibraryCacheManager

79b6ce3

[FLINK-6008] address PR comments

23fb6ec

NicoK force-pushed the flink-6008b branch from 2388986 to 23fb6ec Compare June 30, 2017 14:04

tillrohrmann approved these changes Jul 2, 2017

View reviewed changes

asfgit closed this in 28c3354 Jul 4, 2017

rmetzger added the component=Runtime/Network label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-6008][blob] collection of BlobServer improvements #4146

[FLINK-6008][blob] collection of BlobServer improvements #4146

NicoK commented Jun 20, 2017

NicoK commented Jun 21, 2017

pnowojski Jun 28, 2017

NicoK Jun 28, 2017

pnowojski Jun 29, 2017

pnowojski Jun 28, 2017

NicoK Jun 28, 2017

pnowojski Jun 29, 2017

pnowojski Jun 28, 2017

NicoK Jun 28, 2017

pnowojski Jun 29, 2017

pnowojski Jun 28, 2017

NicoK Jun 28, 2017

pnowojski Jun 29, 2017

NicoK Jun 29, 2017

pnowojski Jun 28, 2017

tillrohrmann left a comment

tillrohrmann Jul 1, 2017

NicoK Jul 3, 2017

tillrohrmann Jul 1, 2017

NicoK Jul 3, 2017

zentol commented Jul 4, 2017

[FLINK-6008][blob] collection of BlobServer improvements #4146

[FLINK-6008][blob] collection of BlobServer improvements #4146

Conversation

NicoK commented Jun 20, 2017

NicoK commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol commented Jul 4, 2017