[Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages #168

smallzhongfeng · 2022-08-19T13:05:47Z

What changes were proposed in this pull request?

From the audit log of HDFS, it can be seen that when the HDFS path of this app was last deleted at 18:00:55, the log in the shuffleServer found that the error about file could not be found, and the file would continue to be written. At last we found that when some appId cache was removed in appIdToStorages, and then HdfsStorageManager calls removeResources will cause storagePath to not be deleted.

Why are the changes needed?

When the cache of appIdToStorages removed, the remote path can be deleted normally.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added ut.

codecov-commenter · 2022-08-19T14:56:39Z

Codecov Report

Merging #168 (8e4c077) into master (74cd40b) will increase coverage by 0.02%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #168      +/-   ##
============================================
+ Coverage     58.29%   58.31%   +0.02%     
- Complexity     1262     1266       +4     
============================================
  Files           158      158              
  Lines          8397     8409      +12     
  Branches        779      782       +3     
============================================
+ Hits           4895     4904       +9     
- Misses         3251     3253       +2     
- Partials        251      252       +1

Impacted Files	Coverage Δ
...che/uniffle/server/storage/HdfsStorageManager.java	`90.00% <83.33%> (-2.69%)`	⬇️
...e/spark/shuffle/reader/RssShuffleDataIterator.java	`88.73% <0.00%> (-0.98%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jerqi · 2022-08-22T02:09:14Z

server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java

+          }
+        } catch (Exception e) {
+          LOG.error("Some error happened when fileSystem got the file status.");
+          e.printStackTrace();


Why do we use printStackTrace, it print the error stack in the stdout, could we print the error stack in the log?

jerqi · 2022-08-22T02:29:35Z

It seems that we only call the method removeResources once, if appIdToStorages was removed because of method removeResources, we won't call the method removeResources any more. It seems that pr can't achieve our aim.

jerqi · 2022-08-22T02:10:37Z

server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java

    if (!appIdToStorages.containsKey(appId)) {
-      String msg = "Can't find HDFS storage for appId[" + appId + "]";
-      LOG.error(msg);
-      // outside should deal with null situation


Why do we remove the comment?

…m pathToStorages

smallzhongfeng · 2022-08-22T06:46:42Z

It seems that we only call the method removeResources once, if appIdToStorages was removed because of method removeResources, we won't call the method removeResources any more. It seems that pr can't achieve our aim.

Because we found the log can't find HDFS storage for appid, it means that appIdToStorages has removed the cache of this app. Because removereSources was called only once, but from the HDFS audit log, it can be seen that the shuffle file was deleted at 18:00:55, but was rewritten at 18:00:57, resulting in the loss of the appId of the cache of appIdToStorages, However, the cached appId of shuffleTaskInfos is still there and cannot be removed. Therefore, this exception occurs when removeResources is scheduled again. Therefore, this PR is mainly used to solve the problem that the path cannot be deleted after the cache of appid by appidtostorages expires.

jerqi · 2022-08-22T06:58:56Z

It seems that we only call the method removeResources once, if appIdToStorages was removed because of method removeResources, we won't call the method removeResources any more. It seems that pr can't achieve our aim.

Because we found the log can't find HDFS storage for appid, it means that appIdToStorages has removed the cache of this app. Because removereSources was called only once, but from the HDFS audit log, it can be seen that the shuffle file was deleted at 18:00:55, but was rewritten at 18:00:57, resulting in the loss of the appId of the cache of appIdToStorages, However, the cached appId of shuffleTaskInfos is still there and cannot be removed. Therefore, this exception occurs when removeResources is scheduled again. Therefore, this PR is mainly used to solve the problem that the path cannot be deleted after the cache of appid by appidtostorages expires.

Why will the method removeResources be scheduled again? It seems that the method removeResources will be called once.

jerqi · 2022-08-22T07:00:52Z

server/src/main/java/org/apache/uniffle/server/storage/HdfsStorageManager.java

-      String msg = "Can't find HDFS storage for appId[" + appId + "]";
-      LOG.error(msg);
-      // outside should deal with null situation
-      // todo: it's better to have a fake storage for null situation


Why do we remove this todo?

smallzhongfeng · 2022-08-22T07:02:30Z

Why will the method removeResources be scheduled again? It seems that the method removeResources will be called once.

You can take a look at this thread clearResourceThread in ShuffleTaskManager. As long as the expiredAppIdQueue still has the app, it will call the logic of removeResources, and then call the removeResources of the HdfsStorageManager.

jerqi · 2022-08-22T07:09:08Z

Why will the method removeResources be scheduled again? It seems that the method removeResources will be called once.

You can take a look at this thread clearResourceThread in ShuffleTaskManager. As long as the expiredAppIdQueue still has the app, it will call the logic of removeResources, and then call the removeResources of the HdfsStorageManager.

I know clearResourceThread's logic. I means that we will call method removeResources once for the one app. Right?

smallzhongfeng · 2022-08-22T07:25:55Z

I know clearResourceThread's logic. I means that we will call method removeResources once for the one app. Right?

Yes, normally, an app calls the logic of removeResources once. However, an exception occurs, which causes the shuffle file to be deleted at 18:00:55 and the file to be recreated at 18:00:57. The expiredAppIdQueue adds the appId again, and then the path of the appId is not deleted.

jerqi · 2022-08-22T07:27:27Z

I know clearResourceThread's logic. I means that we will call method removeResources once for the one app. Right?

Yes, normally, an app calls the logic of removeResources once. However, an exception occurs, which causes the shuffle file to be deleted at 18:00:55 and the file to be recreated at 18:00:57. The expiredAppIdQueue adds the appId again, and then the path of the appId is not deleted.

Why does the expiredAppIdQueue add the appId again?

smallzhongfeng · 2022-08-22T09:39:20Z

This problem was fixed immediately after it appeared last week. Unfortunately, there was no backup log during the redeployment. However, according to the results of last week, we think that the appId was rewritten into the buffer of shuffleTaskInfos at 18:00:57, and the expiredAppIdQueue stores the appId. At present, this HDFS path is also deleted normally.

jerqi · 2022-08-22T09:45:27Z

I just doubt that it's not a root cause. Maybe we can merge this pr first. Please resolve the comment https://github.com/apache/incubator-uniffle/pull/168/files#r951078297

smallzhongfeng · 2022-08-22T11:44:57Z

At present, this is an occasional problem, and the impact is relatively controllable.We will observe it first, and any subsequent progress will be returned to the community. @jerqi

jerqi

Let's merge this pr first. Thanks @smallzhongfeng

smallzhongfeng · 2022-08-22T12:13:39Z

OK, thanks for your patient review. @jerqi

smallzhongfeng changed the title ~~[Improvement] When appId does not exist in appIdToStorages, match from pathToStorages~~ [Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages Aug 19, 2022

smallzhongfeng closed this Aug 19, 2022

smallzhongfeng reopened this Aug 19, 2022

smallzhongfeng force-pushed the ensure-delete-file branch from 12040ca to 19849bb Compare August 19, 2022 14:39

jerqi reviewed Aug 22, 2022

View reviewed changes

[Improvement] When appId does not exist in appIdToStorages, match fro…

478d875

…m pathToStorages

smallzhongfeng force-pushed the ensure-delete-file branch from 4c54fae to 478d875 Compare August 22, 2022 06:47

jerqi reviewed Aug 22, 2022

View reviewed changes

add comment back

8e4c077

jerqi approved these changes Aug 22, 2022

View reviewed changes

jerqi merged commit 805a372 into apache:master Aug 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages #168

[Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages #168

smallzhongfeng commented Aug 19, 2022 •

edited

Loading

codecov-commenter commented Aug 19, 2022 •

edited

Loading

jerqi Aug 22, 2022

smallzhongfeng Aug 22, 2022

jerqi commented Aug 22, 2022

jerqi Aug 22, 2022

smallzhongfeng Aug 22, 2022

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022

jerqi Aug 22, 2022 •

edited

Loading

smallzhongfeng Aug 22, 2022

smallzhongfeng commented Aug 22, 2022 •

edited

Loading

jerqi commented Aug 22, 2022

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022 •

edited

Loading

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022

smallzhongfeng commented Aug 22, 2022

jerqi left a comment

smallzhongfeng commented Aug 22, 2022 •

edited

Loading

[Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages #168

[Improvement] Should match from pathToStorages when appId does not exist in appIdToStorages #168

Conversation

smallzhongfeng commented Aug 19, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Aug 19, 2022 • edited Loading

Codecov Report

jerqi Aug 22, 2022

Choose a reason for hiding this comment

smallzhongfeng Aug 22, 2022

Choose a reason for hiding this comment

jerqi commented Aug 22, 2022

jerqi Aug 22, 2022

Choose a reason for hiding this comment

smallzhongfeng Aug 22, 2022

Choose a reason for hiding this comment

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022

jerqi Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

smallzhongfeng Aug 22, 2022

Choose a reason for hiding this comment

smallzhongfeng commented Aug 22, 2022 • edited Loading

jerqi commented Aug 22, 2022

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022 • edited Loading

smallzhongfeng commented Aug 22, 2022

jerqi commented Aug 22, 2022

smallzhongfeng commented Aug 22, 2022

jerqi left a comment

Choose a reason for hiding this comment

smallzhongfeng commented Aug 22, 2022 • edited Loading

smallzhongfeng commented Aug 19, 2022 •

edited

Loading

codecov-commenter commented Aug 19, 2022 •

edited

Loading

jerqi Aug 22, 2022 •

edited

Loading

smallzhongfeng commented Aug 22, 2022 •

edited

Loading

jerqi commented Aug 22, 2022 •

edited

Loading

smallzhongfeng commented Aug 22, 2022 •

edited

Loading