chore: audit store/streaming/file/service.go

[alexanderbez]

Description

Perform an audit of store/streaming/file/service.go. Changes include cleanup godocs and some linting.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[alexanderbez]

@yihuang I just took a look at the default config and I don't think these should be default:

		Streamers: StreamersConfig{
			File: FileStreamerConfig{
				Keys:            []string{"*"},
				WriteDir:        "data/file_streamer",
				OutputMetadata:  true,
				StopNodeOnError: true,
				// NOTICE: the default config don't protect the streamer data integrity
				// in face of system crash.
				Fsync: false,
			},
		},

This is the default config and as it stands, it means state streaming is enabled by default.

1. I don't think we should have state streaming enabled by default (it should be opt-in). It's more burden on the operator to have to deal with this (more disk space, management, etc...
2. There should be no default WriteDir.

I propose Keys and WriteDir are empty by default. Are you opposed to me making these changes?

[yihuang]

I propose Keys and WriteDir are empty by default. Are you opposed to me making these changes?

No, it's not enabled by default because the store.streamers is empty by default.
The default value of WriteDir set to the same directory as the other dbs, so user just need to add file to store.streamers, then already have a working file streamer.

[alexanderbez]

Ahh I see. It still doesn't make sense to have a default WriteDir as data/file_streamer. It should be explicitly set by the operator. Are you opposed to that?

[yihuang]

Ahh I see. It still doesn't make sense to have a default WriteDir as data/file_streamer. It should be explicitly set by the operator. Are you opposed to that?

I think it provide best UX for user to simply add file into store.streamers to make it work, but I'm ok with either way, it only makes slight difference to me.

[alexanderbez]

Another point @yihuang, I don't see any point in explicit length prefixing. Specifically,

1. For block-%d-data files, there could be multiple entries, but you already call MarshalLengthPrefixed in writeBlockData, so there is no need for an explicit length prefixing in writeLengthPrefixedFile -- it just makes the consumer's life harder and doesn't tell you anything useful about file contents.
2. Same for block-%d-meta files. Since there is only one object per file, BlockMetadata, there is no need for an explicit length-prefix in writeLengthPrefixedFile.

Let me know if I'm wrong on any of these.

[yihuang]

Another point @yihuang, I don't see any point in explicit length prefixing.

The prefix length is to detect file completeness, because we can't guarantee the file writing is atomic, and protobuf encoding won't help with that neither.

[sonarcloud]

[Cosmos SDK] Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
No Duplication information

[julienrbrt]

utACK