HTTP Upload - too much "failed to delete" file warnings

[Canta]

System info

Operating System: Ubuntu 18.04.5 LTS (dockerized)
Shaka Packager Version: 4686454-release

Issue and steps to reproduce the problem

Related issue: #945
This is the same situation, but with other warning I didn't saw before (because my streams have a two hours long time_shift_buffer_depth, and needed to leave the stream online that much).

While using HTTP UPLOAD, shaka packager is constantly writting this kind of messages, flooding the logs:

(...)
[0527/154047:WARNING:representation.cc(466)] Failed to delete http://URL/360_chunkxxx.m4s; Will retry later.
[0527/184107:WARNING:media_playlist.cc(730)] Failed to delete http://URL/480_chunkxxx.m4s; Will retry later.
(...)

That's, at least once, for every chunk, for every rendition, both HLS and MPEG-DASH.

hls/base/media_playlist.cc and mpd/base/representation.cc do check File::Delete output status, and then trigger that warning. But instead of applying changes there, I would like to suggest this other change:

diff --git a/packager/file/file.cc b/packager/file/file.cc
index 01b16f76..4ce0efee 100644
--- a/packager/file/file.cc
+++ b/packager/file/file.cc
@@ -213,9 +213,15 @@ bool File::Delete(const char* file_name) {
   base::StringPiece real_file_name;
   const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name);
   DCHECK(file_type);
-  return file_type->delete_function
-             ? file_type->delete_function(real_file_name.data())
-             : false;
+  if (file_type->delete_function) {
+    return file_type->delete_function(real_file_name.data());
+  } else {
+    VLOG(2) << "File::Delete: file type for "
+            << file_name
+            << " (" << file_type << ") "
+            << "has no 'delete' function.";
+    return true;
+  }
 }

That way, if we need to debug deletes, we can tune up the loglevel for file class, and otherwise filetypes without delete function would not trigger constant warnings down the road.

I would like you oppinion for this @kqyang before commit. I guess it's fine, but I don't know if that behaviour change would trigger any side effects.

Packager Command:

packager 
'in=udp://127.0.0.1:20011?timeout=8000000&buffer_size=10000000,stream_selector=0,segment_template=http://url/720_$Time$.ts,playlist_name=720/720.m3u8' 
'in=udp://127.0.0.1:20011?timeout=8000000&buffer_size=10000000,stream_selector=1,segment_template=http://url/audio_$Time$.ts,playlist_name=audio/audio.m3u8' 
--io_block_size 65536 
--io_cache_size 1000000 
--hls_master_playlist_output "http://url/master.m3u8" 
--hls_playlist_type LIVE 
--default_language=eng 
--dump_stream_info 

What is the expected result?

Just upload using HTTP, without all those warnings.

What happens instead?

A flood of warnings in the logs.

[kqyang]

@Canta Thanks for catching the problem. We should implement HTTP DELETE, which is unfortunately not supported yet.

I suggest using LOG(WARNING) instead of VLOG(2) so it is ignored silently. To avoid log spamming, we can log once only, e.g.

static logged = false;
if (!logged) {
  logged = true;
  LOG(WARNING) << "...";
}

@Canta What do you think?

[Canta]

@kqyang
IDK about the right way, but I feel confident this will not break anything, so already sent a PR.
Tested it successfuly. It just triggers the warning once now.

[Canta]

@kqyang

I can see this ticket was sent to backlog. Yet, I'm about deploy some http uploads to production, and without the fix for the delete warnings merged in the main repo, I'll have to build shaka from my personal fork, which I would like to avoid when dealing with production environments. If the code is OK: perhaps you could reconsider the time window for this merge?

Thanks.

[kqyang]

Ah, sorry for missing the review. I have just merged the change.