dataroaring commented on PR #55751:
URL: https://github.com/apache/doris/pull/55751#issuecomment-3284140395
**⚠️ MAJOR: Hardcoded Magic Number with Security Risk**
**File**: `be/src/olap/tablet.cpp` line 178
```cpp
if (!(injection || (version_count > max_version_config - 100))) {
```
**Issues**:
1. **Magic Number**: The hardcoded `100` has no clear meaning or
documentation
2. **Security Risk**: Your TODO comment mentions "silent errors" if
`max_version_config <= 100`
3. **Maintenance Burden**: Changes to this threshold require code
modifications
**Potential Problems**:
- If someone sets `max_version_config = 50`, this becomes `version_count >
-50` (always true)
- Configuration validation doesn't prevent dangerous values
- No clear rationale for why 100 is the right threshold
**Recommendations**:
**Option 1**: Add validation and use a named constant:
```cpp
static constexpr int MIN_VERSION_THRESHOLD = 100;
static constexpr int VERSION_COMPACTION_BUFFER = 100;
// Add validation earlier:
if (max_version_config <= MIN_VERSION_THRESHOLD) {
return Status::InternalError("max_version_config must be > " +
std::to_string(MIN_VERSION_THRESHOLD));
}
// Use named constant:
if (!(injection || (version_count > max_version_config -
VERSION_COMPACTION_BUFFER))) {
```
**Option 2**: Make it configurable:
```cpp
int compaction_threshold = config::version_compaction_threshold; // Default:
100
if (!(injection || (version_count > max_version_config -
compaction_threshold))) {
```
This prevents silent errors and makes the behavior explicit and configurable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
