Skip to content

Make output directory optional for collector #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ilyaluk
Copy link
Contributor

@ilyaluk ilyaluk commented Jun 25, 2025

📝 Summary

  • Make --out flag optional in CLI configuration
  • Add validation requiring either --out or --clickhouse-dsn to be specified
  • Modify TxProcessor to handle empty OutDir by skipping file operations
  • Refactor error handling to separate validation from file writing
  • Use slices.Contains for cleaner source validation
  • Change use of map[int]*OutFiles to map[int]OutFiles to better handle no-write case

The code and summary was heavily llm-generated and manually reviewed/fixed.

Better viewed with whitespace changes hidden.

⛱ Motivation and Context

We want to deploy collector in k8s and there we don't need to write any files, only write to CH

📚 References


✅ I have run these commands

  • make lint
  • make test
  • go mod tidy

@ilyaluk ilyaluk requested review from metachris and Copilot June 25, 2025 17:32
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new --no-write flag for the mempool collector, allowing file writing to be disabled when not needed.

  • Introduces a WriteFiles field in both TxProcessorOpts and CollectorOpts to conditionally enable CSV file creation.
  • Updates file handling logic in TxProcessor methods (including output file creation, writing, and trash file handling) to respect the WriteFiles flag.
  • Modifies the CLI flags in cmd/collect/main.go to make the --out flag optional when --no-write is set.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
collector/tx_processor.go Adjusts file output logic and changes the use of map[int]*OutFiles to map[int]OutFiles to accommodate the WriteFiles flag.
collector/collector.go Adds the WriteFiles field to CollectorOpts and passes it to the TxProcessor.
cmd/collect/main.go Updates CLI flag definitions to conditionally require the output directory based on the no-write flag.
Comments suppressed due to low confidence (2)

collector/tx_processor.go:55

  • Changing the map from pointers to OutFiles to values may lead to unintended copies of file handle state. Please confirm that file handle management remains safe and that the value semantics do not interfere with expected behavior.
	outFiles     map[int64]OutFiles

collector/tx_processor.go:436

  • Given the change in the return type from a pointer to a value in getOutputCSVFiles, consider updating the function comments/documentation to clarify that a default (zero) OutFiles value is returned on error cases instead of nil.
	outFiles = OutFiles{


outFilesLock sync.RWMutex
outFiles map[int64]*OutFiles
outFiles map[int64]OutFiles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change from pointer to value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly to avoid nil pointer deref, it was a bit more convenient that way in processTx. E.g. we can have zero value of OutFiles, and then reference its fields, just check that fd is not nil.

Also, it's probably easier on memory, storing struct with three pointers in map is faster (I think?) than storing a pointer to a different location on heap. This doesn't matter that much here, though.

@ilyaluk ilyaluk force-pushed the ilya/add-no-write-flag-collector branch from 74168f9 to 234a439 Compare June 26, 2025 12:27
@ilyaluk ilyaluk changed the title Add --no-write flag to disable file output in mempool collector Make output directory optional for collector Jun 26, 2025
@ilyaluk ilyaluk force-pushed the ilya/add-no-write-flag-collector branch from 234a439 to 5a2e757 Compare June 26, 2025 12:33
@ilyaluk ilyaluk requested a review from metachris June 26, 2025 12:33
- Make --out flag optional in CLI configuration
- Add validation requiring either --out or --clickhouse-dsn to be specified
- Modify TxProcessor to handle empty OutDir by skipping file operations
- Refactor error handling to separate validation from file writing
- Use slices.Contains for cleaner source validation
@ilyaluk ilyaluk force-pushed the ilya/add-no-write-flag-collector branch from 5a2e757 to 65f8032 Compare June 26, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants