fgerlits commented on code in PR #2148:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2148#discussion_r3009249382
##########
extensions/libarchive/FocusArchiveEntry.cpp:
##########
@@ -181,12 +181,7 @@ int64_t FocusArchiveEntry::ReadCallback::operator()(const
std::shared_ptr<io::In
if (res < ARCHIVE_OK) {
logger_->log_error("FocusArchiveEntry can't read header due to archive
error: {}", archive_error_string(input_archive.get()));
- return nlen;
- }
-
- if (res < ARCHIVE_WARN) {
- logger_->log_warn("FocusArchiveEntry got archive warning while reading
header: {}", archive_error_string(input_archive.get()));
- return nlen;
+ return io::IoResult::error();
Review Comment:
I would keep this as `zero()` so that we don't change the logic, unless you
understand it fully and you're sure that it was wrong.
Also, I would fix the warning vs error distinction instead of removing it:
* check for errors first, log error, return zero
* then check for retry/warning, log retry/warning, return zero.
##########
extensions/libarchive/FocusArchiveEntry.cpp:
##########
@@ -169,7 +169,7 @@ int64_t FocusArchiveEntry::ReadCallback::operator()(const
std::shared_ptr<io::In
// Read each item in the archive
if (archive_read_open(input_archive.get(), &data, ok_cb, read_cb, ok_cb)) {
logger_->log_error("FocusArchiveEntry can't open due to archive error:
{}", archive_error_string(input_archive.get()));
- return nlen;
+ return io::IoResult::fromI64(nlen);
Review Comment:
```suggestion
return io::IoResult::zero();
```
since there is no way for `nlen` to be anything other than zero here
##########
extensions/rocksdb-repos/tests/SwapTests.cpp:
##########
@@ -56,14 +56,14 @@ class OutputProcessor : public core::ProcessorImpl {
void onTrigger(core::ProcessContext&, core::ProcessSession& session)
override {
auto id = std::to_string(next_id_++);
- auto ff = session.create();
+ const auto ff = session.create();
ff->addAttribute("index", id);
- session.write(ff, [&] (const std::shared_ptr<minifi::io::OutputStream>&
output) -> int64_t {
- auto ret = output->write(as_bytes(std::span(id)));
- if (minifi::io::isError(ret)) {
- return -1;
+ session.write(ff, [&] (const std::shared_ptr<minifi::io::OutputStream>&
output) -> io::IoResult {
+ const size_t ret = output->write(as_bytes(std::span(id)));
+ if (io::isError(ret)) {
+ return io::IoResult::error();
}
- return gsl::narrow<int64_t>(ret);
+ return io::IoResult::fromSizeT(ret);
Review Comment:
nitpicking, but fromSizeT checks for error internally, so lines 63-65 can be
removed
##########
libminifi/src/core/ProcessSession.cpp:
##########
@@ -258,14 +258,14 @@ void ProcessSessionImpl::write(core::FlowFile &flow,
const io::OutputStreamCallb
throw Exception(FILE_OPERATION_EXCEPTION, "Failed to open flowfile
content for write");
}
const auto callback_result = callback(stream);
- if (callback_result == MinifiIoStatus::MINIFI_IO_CANCEL) {
- stream->close();
- content_session_->remove(claim);
- claim.reset();
- return;
- }
+ if (!callback_result) {
+ if (callback_result.is_cancelled()) {
+ stream->close();
+ content_session_->remove(claim);
+ claim.reset();
+ return;
+ }
- if (callback_result < 0) {
throw Exception(FILE_OPERATION_EXCEPTION, "Failed to process flowfile
content");
}
Review Comment:
I liked the previous flow better, without nesting the `if`s.
##########
libminifi/src/sitetosite/CompressionOutputStream.cpp:
##########
@@ -103,7 +103,7 @@ size_t CompressionOutputStream::compressAndWrite() {
}
// Write the compressed data
- ret = internal::pipe(buffer_stream, internal_stream_);
+ ret = minifi::io::IoResult(internal::pipe(buffer_stream,
internal_stream_)).toI64(); // TODO(mzink) feels wrong
Review Comment:
Yes, it does... Maybe we could add a `toSizeT` method? Or keep the return
value of `pipe` as `size_t` and convert it later?
(Also, the copy seems unnecessary.)
##########
minifi-api/common/include/minifi-cpp/io/StreamCallback.h:
##########
@@ -16,24 +16,101 @@
*/
#pragma once
+#include <cinttypes>
#include <functional>
#include <memory>
#include <optional>
Review Comment:
`#include <optional>` can be removed
##########
minifi-api/common/include/minifi-cpp/io/StreamCallback.h:
##########
@@ -16,24 +16,101 @@
*/
#pragma once
+#include <cinttypes>
#include <functional>
#include <memory>
#include <optional>
+#include "../../minifi-api/include/minifi-c/minifi-c.h"
Review Comment:
elsewhere we have
```suggestion
#include "minifi-c/minifi-c.h"
```
would that work here?
--
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]
