Walkthrough

The scheduler’s filesystem input processing now validates and buffers paths while collecting invalid-path messages. A new helper writes these messages to a per-job user log. The main search-and-schedule flow handles FS invalid paths, writes the log, marks jobs FAILED, broadens S3 error handling, and adjusts buffer flush/sequence.

Changes

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant S as Scheduler
  participant FS as _process_fs_input_paths
  participant L as _write_failed_path_log
  participant J as JobStore

  rect rgba(230,245,255,0.6)
  note over S: FS input handling (updated)
  S->>FS: Validate and buffer FS paths
  FS-->>S: invalid_path_messages (List[str])
  alt invalid_path_messages not empty
    S->>L: Write failed paths log (messages, logs_dir, job_id)
    L-->>S: log_path or None
    S->>J: Update job -> FAILED with status_msg (log_path or scheduler logs path)
    S->>J: Commit
    S-->>S: Continue to next job
  else no invalid messages
    S->>S: Flush buffer, collect tasks/partitions
    note over S: Proceed with scheduling flow
  end
  end

sequenceDiagram
  autonumber
  participant S as Scheduler
  participant S3 as _process_s3_input
  participant J as JobStore

  rect rgba(255,240,230,0.6)
  note over S: S3 input handling (broadened)
  S->>S3: Process S3 input
  alt success
    S-->>S: Continue normal scheduling
  else exception
    S3-->>S: throw Error
    S->>J: Update job -> FAILED with "S3 Failure: {err}"
    S->>J: Commit
    S-->>S: Continue to next job
  end
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai review

Quick question: what would appear on webui if a job partially fail?

Edit: I guess this is irrelevant now since a job will either pass or fail.

@haiqi96 and @kirkrodrigues, here are my thoughts on what I should change re. catching the error:

I could omit logger.error(str(ex)) from _process_fs_input_paths, and then it would be logged only once (at the caller around line 211)

OR

I could omit logger.exception(error_msg) (at the caller around line 211) and then it would still be logged only once (in process_fs_input_paths).

Not sure about the whole "double-raising" thing; I don't know how else to pass the error message from validate_path_and_get_info all the way to the caller around line 211.

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).

Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).

Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).

Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

I am not against it. in that case, the _process_fs_input_paths method needs to somehow let caller knows what path has failed. I have the following proposal:

1. _process_fs_input_paths() shall return a List[str] of failing path. an empty list means no failure. Alternatively, it can return a Tuple of (success, failing_path): [bool, List[str]]
2. The caller checks if _process_fs_input_paths succeed. if there's any failing path:
   • It will locally generate a log file, where each line of the log file is a failing path.
   • it shall mark the job as failed without proceeding, with a short error message stating that (1) the job failed because of invalid input path(s), (2), point the user to the generated log file on the disk

@kirkrodrigues any comment?

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).
Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).
Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

I am not against it. in that case, the _process_fs_input_paths method needs to somehow let caller knows what path has failed. I have the following proposal:

1. _process_fs_input_paths() shall return a List[str] of failing path. an empty list means no failure. Alternatively, it can return a Tuple of (success, failing_path): [bool, List[str]]

2. The caller checks if _process_fs_input_paths succeed. if there's any failing path:
   
   * It will locally generate a log file, where each line of the log file is a failing path.
   * it shall mark the job as failed without proceeding, with a short error message stating that (1) the job failed because of invalid input path(s), (2), point the user to the generated log file on the disk

@kirkrodrigues any comment?

Sure

@haiqi96 and @kirkrodrigues, I’ve pushed a new implementation. The core elements of the new strategy are a combination of your proposals, as follows:

1. Don’t fail compression jobs upon encountering an invalid path; complete the compression job if there is at least one valid path
2. Log the failing paths somewhere
3. Inform the user to check the logs for which paths failed

That being said, I felt that part of the solution that Haiqi proposed, i.e.

_process_fs_input_paths() shall return a List[str] of failing path

could instead be fulfilled by the logging infrastructure already used in this function (the failed path is already being recorded to the compression_scheduler log, which can be viewed by the user). With that in mind, it makes more sense to me to return bool from _process_fs_input_paths; if we are simply reporting to the user that at least one path failed, and telling them to check the logs for the failed paths, then the caller of _process_fs_input_paths doesn’t need to know the path(s) that failed, as the failing path(s) have already been logged in a way that the user can access.

Let me know what you think.

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).
Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

Sorry, just getting to this now. I feel failing on the first invalid path is a bit harsh since a user might be compressing a directory with files that change or get deleted (e.g., files in /var/log during log rotation).
Ideally, we should report a partial error with the job and what paths failed, so that the user could retry just those paths if they like (and maybe one day we can even support retrying the files at the click of a button). However, before then, I think a reasonable compromise is to log the failing paths and inform the user to check the logs for which paths failed. What do y'all think?

I am not against it. in that case, the _process_fs_input_paths method needs to somehow let caller knows what path has failed. I have the following proposal:

1. _process_fs_input_paths() shall return a List[str] of failing path. an empty list means no failure. Alternatively, it can return a Tuple of (success, failing_path): [bool, List[str]]

2. The caller checks if _process_fs_input_paths succeed. if there's any failing path:
   
   * It will locally generate a log file, where each line of the log file is a failing path.
   * it shall mark the job as failed without proceeding, with a short error message stating that (1) the job failed because of invalid input path(s), (2), point the user to the generated log file on the disk

@kirkrodrigues any comment?

Sure

@haiqi96 and @kirkrodrigues, I’ve pushed a new implementation. The core elements of the new strategy are a combination of your proposals, as follows:

1. Don’t fail compression jobs upon encountering an invalid path; complete the compression job if there is at least one valid path
2. Log the failing paths somewhere
3. Inform the user to check the logs for which paths failed

That being said, I felt that part of the solution that Haiqi proposed, i.e.

_process_fs_input_paths() shall return a List[str] of failing path

could instead be fulfilled by the logging infrastructure already used in this function (the failed path is already being recorded to the compression_scheduler log, which can be viewed by the user). With that in mind, it makes more sense to me to return bool from _process_fs_input_paths; if we are simply reporting to the user that at least one path failed, and telling them to check the logs for the failed paths, then the caller of _process_fs_input_paths doesn’t need to know the path(s) that failed, as the failing path(s) have already been logged in a way that the user can access.

Let me know what you think.

I don't agree with the approach of logging the paths in compression_scheduer.

1. It's easier to point user to a specific path, rather than saying "check compression_scheduer log" since most users (and even some people in our team who don't use package frequently) don't know where is compression_scheduer's log.
2. Imagine if the compression scheduler has been running for a while and logged many different things, it would be hard for user to find the exact portion of logs that he/she should read.

Because of the points above, it would makes sense to generate a separate log file for clear/efficient error reporting.

most users (and even some people in our team who don't use package frequently) don't know where is compression_scheduer's log

@haiqi96 that's true. But you also mention:

It's easier to point user to a specific path

... I could rig it so that it prints out the specific path of the compression_scheduler log for the user?

As for this point:

it would be hard for user to find the exact portion of logs that he/she should read

that's fair.

@haiqi96, I guess my objection to your proposal that "_process_fs_input_paths() shall return a List[str]" lies in the fact that the paths are already being logged in the compression_scheduler logs; my approach just directs the user there.

Also, see my comment re. the "immediate failure" vs "continue compression" discussion.