Implement run-length encoding and aggregate function with concurrent #24
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements three main tasks: run-length encoding, a race-free ID generator, and a concurrent file stats aggregator with per-file timeouts.
- Added
Encodefor run-length encoding inrle.go. - Switched
NextIDto atomic operations inbuggy_counter.go. - Built out
Aggregate,job,worker, andreadFileinaggregator.gofor concurrent processing with timeouts.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tasks/01-run-length/go/rle.go | Implement Encode using fmt.Sprintf to produce RLE of a string. |
| tasks/02-fix-the-bug/go/buggy_counter.go | Replace non-atomic increment with atomic.AddInt64 to avoid races. |
| tasks/03-sync-aggregator/go/aggregator.go | Fully implement Aggregate, spawning workers, job dispatch, and per-file timeouts. |
Comments suppressed due to low confidence (2)
tasks/01-run-length/go/rle.go:14
- [nitpick] The variable name
sris not very descriptive. Consider renaming it to something likerunesorcharsfor clarity.
sr := []rune(s)
tasks/03-sync-aggregator/go/aggregator.go:24
- This new concurrent aggregator logic would benefit from unit tests covering normal operation, timeout behavior, and error cases to ensure correctness.
func Aggregate(filelistPath string, workers, timeout int) ([]Result, error) {
|
|
||
| func readFile(ctx context.Context, path string) (int, int, error) { | ||
| filelist := filepath.Join("..", "data", path) | ||
| f, _ := os.Open(filelist) |
There was a problem hiding this comment.
You’re ignoring the error from os.Open. It’s safer to capture and handle it (e.g., f, err := os.Open(...); if err != nil { return 0, 0, err }).
| f, _ := os.Open(filelist) | |
| f, err := os.Open(filelist) | |
| if err != nil { | |
| return 0, 0, err | |
| } |
| line := scanner.Text() | ||
| if strings.HasPrefix(line, "#sleep=") { | ||
| sleepDur := strings.TrimPrefix(line, "#sleep=") | ||
| sleepSec, _ := time.ParseDuration(sleepDur + "s") |
There was a problem hiding this comment.
Parsing durations without checking for errors can mask malformed input. Consider handling the parse error to avoid unexpected zero durations.
| sleepSec, _ := time.ParseDuration(sleepDur + "s") | |
| sleepSec, err := time.ParseDuration(sleepDur + "s") | |
| if err != nil { | |
| return lineCount, wordCount, err | |
| } |
| index, _ := fileIndex[res.Path] | ||
| output[index] = res |
There was a problem hiding this comment.
You’re ignoring the lookup result when indexing fileIndex. If res.Path isn’t present, you’ll default to zero and could overwrite the wrong slot. Check ok or handle missing paths explicitly.
| index, _ := fileIndex[res.Path] | |
| output[index] = res | |
| if index, exists := fileIndex[res.Path]; exists { | |
| output[index] = res | |
| } else { | |
| // Handle missing path explicitly (e.g., log an error or skip) | |
| // Uncomment the following line to log the error: | |
| // fmt.Printf("Warning: Path %s not found in fileIndex\n", res.Path) | |
| } |
| if len(s) == 0 { | ||
| return "" | ||
| } | ||
| result := "" |
There was a problem hiding this comment.
[nitpick] Repeated string concatenation with fmt.Sprintf can be inefficient. Consider using strings.Builder to minimize allocations.
| id := current | ||
| // use atomic to add a current to avoid a race | ||
| id := atomic.AddInt64(¤t, 1) - 1 | ||
| time.Sleep(0) |
There was a problem hiding this comment.
[nitpick] time.Sleep(0) after an atomic increment is likely unnecessary. You can remove it or add a comment explaining its purpose.
| time.Sleep(0) |
|
ping @Aorts thank you for your PR. I just want to confirm that the task 3's test is failing and whether you want us to review the PR as-is, or you want to fix it before the deadline. thank you 🙏 |
2 participants
No description provided.