HDFS-16967. RBF: File based state stores should allow concurrent access to the records #5523
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ss to the records
|
Based on one of the testing data points (hdfs as state store driver), for the same num of mount table records to be loaded in the cache, avg time taken by default is ~1500 ms whereas with concurrent mode, it goes down to ~130 ms. |
|
🎊 +1 overall
This message was automatically generated. |
|
Default mode:
Concurrent access mode:
|
goiri
approved these changes
Mar 31, 2023
| @@ -168,9 +182,30 @@ public boolean initDriver() { | |||
| return false; | |||
| } | |||
| setInitialized(true); | |||
| int threads = getConcurrentFilesAccessNumThreads(); | |||
| if (threads > 0) { | |||
There was a problem hiding this comment.
Should it be >1?
Technically 1 thread would be serial.
.../java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreFileBaseImpl.java
Show resolved
Hide resolved
| Configuration conf = | ||
| FederationStateStoreTestUtils.getStateStoreConfiguration(StateStoreFileSystemImpl.class); | ||
| conf.set(StateStoreFileSystemImpl.FEDERATION_STORE_FS_PATH, "/hdfs-federation/"); | ||
| conf.set(FEDERATION_STORE_FS_ASYNC_THREADS, numFsAsyncThreads); |
There was a problem hiding this comment.
Could we make it setInt() and pass the number as an int.
It would be cleaner.
I'm not sure how the Parameterized handles that though.
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Apr 3, 2023
simbadzina
reviewed
Apr 3, 2023
| return new QueryResult<>(result, getTime()); | ||
| } | ||
|
|
||
| private <T extends BaseRecord> Void getRecordsFromFileAndRemoveOldTmpRecords(Class<T> clazz, |
There was a problem hiding this comment.
Can you add documentation to this function indication that the results list is being modified to collect the results.
Changing the function name would make that clearer too.
There was a problem hiding this comment.
Done, thanks.
Since the method name already seems long enough, directly added to the Javadoc. Sounds good?
| return success.get(); | ||
| } | ||
|
|
||
| private <T extends BaseRecord> Void writeRecordToFile(AtomicBoolean success, |
There was a problem hiding this comment.
Similar comment as above. Can we add documentation indicating success is being modified.
| @@ -137,6 +149,8 @@ public abstract <T extends BaseRecord> BufferedWriter getWriter( | |||
| */ | |||
| protected abstract String getRootDir(); | |||
|
|
|||
| protected abstract int getConcurrentFilesAccessNumThreads(); | |||
There was a problem hiding this comment.
Can we provide an implementation here and then just have one set of configs for the following two
FEDERATION_STORE_PREFIX + "driver.file.async.threads";
FEDERATION_STORE_PREFIX + "driver.fs.async.threads";
I'm okay with keeping them separate though if you have prefer that.
There was a problem hiding this comment.
I believe separate configs would be better since we anyways have different implementation for many of other methods too. Sounds good to keep it as is?
There was a problem hiding this comment.
Yeah, good to keep it as it is.
|
Thanks @simbadzina, addressed your comments |
simbadzina
approved these changes
Apr 3, 2023
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for your contribution, @virajjasani. |
ferdelyi
pushed a commit
to ferdelyi/hadoop
that referenced
this pull request
May 26, 2023
…ss to the records (apache#5523) Reviewed-by: Inigo Goiri <inigoiri@apache.org> Reviewed-by: Simbarashe Dzinamarira <sdzinamarira@linkedin.com> Signed-off-by: Takanobu Asanuma <tasanuma@apache.org>
NyteKnight
pushed a commit
to NyteKnight/hadoop
that referenced
this pull request
Jun 25, 2024
…ss to the records (apache#5523) Reviewed-by: Inigo Goiri <inigoiri@apache.org> Reviewed-by: Simbarashe Dzinamarira <sdzinamarira@linkedin.com> Signed-off-by: Takanobu Asanuma <tasanuma@apache.org> (cherry-picked from 937caf7) ACLOVERRIDE
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
File based state store implementations (StateStoreFileImpl and StateStoreFileSystemImpl) should allow updating as well as reading of the state store records concurrently rather than serially. Concurrent access to the record files on the hdfs based store seems to be improving the state store cache loading performance by more than 10x.
For instance, in order to maintain data integrity, when any mount table record(s) is updated, the cache is reloaded. This reload operation seems to be able to gain significant performance improvement by the concurrent access of the mount table records.