lordgamez commented on a change in pull request #917:
URL: https://github.com/apache/nifi-minifi-cpp/pull/917#discussion_r502372984
##########
File path: extensions/libarchive/BinFiles.h
##########
@@ -146,36 +146,23 @@ class Bin {
// BinManager Class
class BinManager {
public:
- // Constructor
- /*!
- * Create a new BinManager
- */
- BinManager()
- : minSize_(0),
- maxSize_(ULLONG_MAX),
- maxEntries_(INT_MAX),
- minEntries_(1),
- binAge_(ULLONG_MAX),
- binCount_(0),
- logger_(logging::LoggerFactory<BinManager>::getLogger()) {
- }
virtual ~BinManager() {
purge();
}
void setMinSize(const uint64_t &size) {
- minSize_ = size;
+ minSize_ = {size};
}
void setMaxSize(const uint64_t &size) {
- maxSize_ = size;
+ maxSize_ = {size};
}
- void setMaxEntries(const int &entries) {
- maxEntries_ = entries;
+ void setMaxEntries(const uint32_t &entries) {
+ maxEntries_ = {entries};
}
- void setMinEntries(const int &entries) {
- minEntries_ = entries;
+ void setMinEntries(const uint32_t &entries) {
+ minEntries_ = {entries};
}
void setBinAge(const uint64_t &age) {
- binAge_ = age;
+ binAge_ = {age};
Review comment:
- Could you please tell me what's the added value of using braces?
According to godbolt there is no difference after compilation.
- I think we could pass the arguments by values instead of reference here.
##########
File path: extensions/libarchive/CompressContent.cpp
##########
@@ -45,11 +45,11 @@ core::Property CompressContent::CompressFormat(
core::PropertyBuilder::createProperty("Compression
Format")->withDescription("The compression format to use.")
->isRequired(false)
->withAllowableValues<std::string>({
- COMPRESSION_FORMAT_ATTRIBUTE,
- COMPRESSION_FORMAT_GZIP,
- COMPRESSION_FORMAT_BZIP2,
- COMPRESSION_FORMAT_XZ_LZMA2,
-
COMPRESSION_FORMAT_LZMA})->withDefaultValue(COMPRESSION_FORMAT_ATTRIBUTE)->build());
+ toString(ExtendedCompressionFormat::USE_MIME_TYPE),
+ toString(CompressionFormat::GZIP),
+ toString(CompressionFormat::BZIP2),
+ toString(CompressionFormat::XZ_LZMA2),
+
toString(CompressionFormat::LZMA)})->withDefaultValue(toString(ExtendedCompressionFormat::USE_MIME_TYPE))->build());
Review comment:
Could we maybe add a function to generate the allowable values? That way
we would only need to define the list in a single place.
##########
File path: extensions/libarchive/CompressContent.h
##########
@@ -33,19 +33,14 @@
#include "core/Property.h"
#include "core/logging/LoggerConfiguration.h"
#include "io/ZlibStream.h"
+#include "utils/Enum.h"
namespace org {
namespace apache {
namespace nifi {
namespace minifi {
namespace processors {
-#define COMPRESSION_FORMAT_ATTRIBUTE "use mime.type attribute"
-#define COMPRESSION_FORMAT_GZIP "gzip"
-#define COMPRESSION_FORMAT_BZIP2 "bzip2"
-#define COMPRESSION_FORMAT_XZ_LZMA2 "xz-lzma2"
-#define COMPRESSION_FORMAT_LZMA "lzma"
-
#define MODE_COMPRESS "compress"
#define MODE_DECOMPRESS "decompress"
Review comment:
These defines could also be moved to static constants
##########
File path: extensions/libarchive/CompressContent.cpp
##########
@@ -60,10 +60,29 @@ core::Property CompressContent::EncapsulateInTar(
"If false, on compression the content of the
FlowFile simply gets compressed, and on decompression a simple compressed
content is expected.\n"
"true is the behaviour compatible with older MiNiFi
C++ versions, false is the behaviour compatible with NiFi.")
->isRequired(false)->withDefaultValue<bool>(true)->build());
+core::Property CompressContent::BatchSize(
+ core::PropertyBuilder::createProperty("Batch Size")
+ ->withDescription("Maximum number of FlowFiles processed in a single
session")
+ ->withDefaultValue<uint32_t>(1)->build());
core::Relationship CompressContent::Success("success", "FlowFiles will be
transferred to the success relationship after successfully being compressed or
decompressed");
core::Relationship CompressContent::Failure("failure", "FlowFiles will be
transferred to the failure relationship if they fail to compress/decompress");
+std::map<std::string, CompressContent::CompressionFormat>
CompressContent::compressionFormatMimeTypeMap_{
Review comment:
These 2 maps could be constants. Also just minor comment, but maybe the
naming could be changed as well to snake_case to comply with the guideline.
##########
File path: extensions/libarchive/CompressContent.cpp
##########
@@ -81,39 +101,42 @@ void CompressContent::initialize() {
}
void CompressContent::onSchedule(core::ProcessContext *context,
core::ProcessSessionFactory *sessionFactory) {
- std::string value;
context->getProperty(CompressLevel.getName(), compressLevel_);
context->getProperty(CompressMode.getName(), compressMode_);
- context->getProperty(CompressFormat.getName(), compressFormat_);
+
+ {
+ std::string compressFormatStr;
+ context->getProperty(CompressFormat.getName(), compressFormatStr);
+ std::transform(compressFormatStr.begin(), compressFormatStr.end(),
compressFormatStr.begin(), ::tolower);
+ compressFormat_ =
ExtendedCompressionFormat::parse(compressFormatStr.c_str());
+ if (!compressFormat_) {
+ throw Exception(PROCESS_SCHEDULE_EXCEPTION, "Unknown compression format:
\"" + compressFormatStr + "\"");
+ }
+ }
Review comment:
Maybe this could be extracted to a separate function as it already has
its own scope.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
