HPCC-35200: Make modeling of index event read times optional #20539
Conversation
Update the index event model configuration: - `storage` is an optional element; and - `storage/plane` is an optional element; and - `storage/file`, already optional, is allowed only when `storage/plane` is defined.
There was a problem hiding this comment.
Pull Request Overview
This PR makes the storage configuration optional in the index event model, allowing the model to work without storage plane definitions. When storage configuration is absent, read times can be provided directly in events via the ReadTime attribute.
Key changes:
- Storage configuration becomes optional at multiple levels (storage element, plane element, and their interdependencies)
- When no storage planes are configured, the model extracts read times directly from event attributes
- Added validation to prevent invalid configurations (e.g., file configurations without plane definitions)
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| eventindexmodelstorage.cpp | Implements optional storage modeling by checking for empty planes, extracting read times from events when no planes exist, and adding validation to prevent file configs without planes |
| eventindexmodel.hpp | Updates method signature to pass full event object instead of individual attributes, removes outdated configuration documentation |
| eventindexmodel.cpp | Makes storage configuration optional, updates method calls to pass event objects, adds three test cases for no-storage scenarios |
| README.md | Updates documentation to reflect optional storage and plane elements with proper constraints |
|
|
||
| void Storage::observeFile(__uint64 fileId, const char* path) | ||
| { | ||
| // don't observe files when no modeling is configured |
There was a problem hiding this comment.
The comment is misleading. The check planes.empty() only verifies that no storage planes are configured, not that modeling is entirely disabled. Memory modeling could still be active. Consider: 'don't observe files when no storage planes are configured'.
| // don't observe files when no modeling is configured | |
| // don't observe files when no storage planes are configured |
| { | ||
| bool haveDefault = false; | ||
| Owned<IPropertyTreeIterator> it = config.getElements("file"); | ||
| if (it->first() && planes.empty()) |
There was a problem hiding this comment.
The call to it->first() consumes the iterator position. This means the subsequent ForEach(*it) at line 158 will skip the first file element. The check should use it->isValid() after calling first() and then reset the iterator, or restructure to avoid consuming the iterator.
| if (it->first() && planes.empty()) | |
| if (it->isValid() && planes.empty()) |
| if (!node) | ||
| throw makeStringException(-1, "index event model configuration missing required <storage> element"); | ||
| storage.configure(*node); | ||
| if (node) |
There was a problem hiding this comment.
[nitpick] The change makes storage optional, but there's no corresponding update to handle the case where storage is present but empty (e.g., <storage/>). While the code may handle this through planes.empty() checks in Storage, it would be clearer to explicitly document this behavior or add a validation that an empty storage element is equivalent to omission.
| if (node) | |
| // Treat an empty <storage/> element as equivalent to omission. | |
| if (node && (node->hasChildren() || node->getNumProps() > 0)) |
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-35200 Jirabot Action Result: |
1 participant
Update the index event model configuration:
storageis an optional element; andstorage/planeis an optional element; andstorage/file, already optional, is allowed only whenstorage/planeis defined.Type of change:
Checklist:
Smoketest:
Testing: