fgerlits commented on code in PR #1499:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1499#discussion_r1305790060
##########
conf/minifi.properties:
##########
@@ -38,6 +38,9 @@ nifi.content.repository.class.name=DatabaseContentRepository
# nifi.flowfile.repository.rocksdb.compaction.period=2 min
# nifi.database.content.repository.rocksdb.compaction.period=2 min
+# setting this value to 0 enables synchronous deletion
Review Comment:
is "0" allowed? if not, then
```suggestion
# setting this value to 0 sec enables synchronous deletion
```
would be better
##########
CONFIGURE.md:
##########
@@ -224,6 +224,17 @@ Finally, as the last line is commented out, it will make
the state manager use p
When multiple repositories use the same directory (as with `minifidb://`
scheme) they should either be all plaintext or all encrypted with the same key.
+### Configuring Repository Cleanup
+
+When a flow file content is no longer needed we can specify the deletion
strategy.
+
+ # any value other than 0 enables async cleanup with the specified period
Review Comment:
I think
```suggestion
# any value other than 0 enables garbage collection with the specified
frequency
```
would be clearer to everyone except physicists (who would complain that it's
the reciprocal of frequency, but I think we can ignore them)
##########
extensions/rocksdb-repos/DatabaseContentRepository.cpp:
##########
@@ -198,27 +212,63 @@ bool DatabaseContentRepository::exists(const
minifi::ResourceClaim &streamId) {
}
bool DatabaseContentRepository::removeKey(const std::string& content_path) {
- if (!is_valid_ || !db_) {
- logger_->log_error("DB is not valid, could not delete %s", content_path);
- return false;
- }
- auto opendb = db_->open();
- if (!opendb) {
- logger_->log_error("Could not open DB, did not delete %s", content_path);
- return false;
- }
- rocksdb::Status status;
- status = opendb->Delete(rocksdb::WriteOptions(), content_path);
- if (status.ok()) {
- logger_->log_debug("Deleting resource %s", content_path);
- return true;
- } else if (status.IsNotFound()) {
- logger_->log_debug("Resource %s was not found", content_path);
- return true;
- } else {
- logger_->log_error("Attempted, but could not delete %s", content_path);
- return false;
+ if (purge_period_ == std::chrono::seconds(0)) {
+ if (!is_valid_ || !db_)
+ return false;
+ // synchronous deletion
+ auto opendb = db_->open();
+ if (!opendb) {
+ return false;
+ }
+ rocksdb::Status status = opendb->Delete(rocksdb::WriteOptions(),
content_path);
+ if (status.ok()) {
+ logger_->log_debug("Deleting resource %s", content_path);
+ return true;
+ } else if (status.IsNotFound()) {
+ logger_->log_debug("Resource %s was not found", content_path);
+ return true;
+ } else {
+ logger_->log_debug("Attempted, but could not delete %s", content_path);
+ return false;
+ }
}
Review Comment:
Nitpicking, but I would make the inside of this `if` into a separate
function. This way, we have to make sure to have a `return` at the end of
every code path; that way, the compiler would check it for us.
--
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]
