to prevent file cleaning, touch cache, its link, md5 and id, resolves #1282 #1367
Conversation
MontrealSergiy
added
Priority: Normal
Admin
MontrealSergiy
changed the title
MontrealSergiy
force-pushed
the
cache_touch#1282
branch
from
5a80441 to
fe078ea
Compare
|
I believe I added the requested changes |
|
For the GridShare dir and the DP cache symlink, I'd rather both 'touch' commands to be run in the bourreau system check in a100_ensure_dp_cache_symlink_exists because this is the ideal place. For the remaining three DataProvider cache files, the class method in DataProvider is fine as it is (minus the code moved to the place I talked about at the beginning of this comment). |
MontrealSergiy
force-pushed
the
cache_touch#1282
branch
from
f9e5bee to
e38da4e
Compare
|
|
||
| sym_path = "#{gridshare_dir}/#{DataProvider::DP_CACHE_SYML}" | ||
| return if File.symlink?(sym_path) && File.realpath(sym_path) == File.realpath(cache_dir) | ||
| # update the work directory timestamp to counter cluster deletion policies |
There was a problem hiding this comment.
Why is this so complicated?!??
The original code's logic was:
return if symlink_is_ok
create symlink
The new code should simply be instead:
if symlink_is_ok
touch symlink
return
end
create symlink
But you did a complex set of conditions including checking the return status of system():
touch_but_not_create symlink
ret=system(touch symlink)
return if ret is 0 and symlink_is_ok
create_symlink
Why is make it so complicated?
There was a problem hiding this comment.
To make code work even if touch fails. But ok, can roll back that check.
| # - the +DataProvider+ cache dir | ||
| # - the +DP_Cache_Key.md5+ and | ||
| # - +DP_Cache_Rev.id+ located in that cache dir | ||
| def self.system_touch |
There was a problem hiding this comment.
The method name system_touch is way too general. The method touches two special config files related to the cache subsystem. In general, in the DataProvider class, all methods that deal with the cache subsystem have "cache_" in their names. So please rename this
def self.touch_cache_md5and place it after create_cache_md5 where it belongs.
|
|
||
| # update timestamp for the symbolic link rather than the folder it points to | ||
| # --no-dereference works on most major os, otherwise link recreated below | ||
| link_updated = system("touch", "--no-dereference", sym_path) |
There was a problem hiding this comment.
Adendum, --no-derefence doesn't work on Darwin.
The options -h works on both Darwin and Linux
| sym_path = "#{gridshare_dir}/#{DataProvider::DP_CACHE_SYML}" | ||
| return if File.symlink?(sym_path) && File.realpath(sym_path) == File.realpath(cache_dir) | ||
| # update the work directory timestamp to counter cluster deletion policies | ||
| FileUtils.touch(gridshare_dir, verbose: true, nocreate: true) |
There was a problem hiding this comment.
Why did you leave the verbose and nocreate option there? Did you check the boot logs? I now see this crud:
C> Trying to see if there are any spurious task work directories...
C> - No spurious task work directories detected.
C> Making sure the grid share directory has a symlink to the data provider cache...
touch -c /Users/prioux/LargeData/BourreauGridshare
C> Ensuring git commits for tasks classes are pre-cached...
C> Making sure the portal is forwarding a SSH agent to us...
C> - Found a forwarded agent on SOCK=/tmp/ssh-8bZbwKlpmA/agent.13854
|
I am closing this PR without merging because it's taking too long with too many back and forth. I replaced it with my own pull request, #1487 which shows how it's supposed to be done. |
|
I didn’t realize you wanted to remove those as well. I used nocreate because the assignment was to update the timestamp, not to recreate folders. Additionally, I believe recreating the shared work could mask problems—it might allow the system to process new tasks but could obscure the need to restore all the other tasks. As for the verbatim option, it provides more details in the logs. I probably didn’t check the log, but I wasn’t surprised by the results. I assumed it was more important to retain details, and I expected you to let me know if it was redundant. |
2 participants
resolves #1282
It maybe safer to recreate the symlink than touch, but I guess should work for us