exceptionfactory commented on code in PR #11107:
URL: https://github.com/apache/nifi/pull/11107#discussion_r3040852371
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -50,7 +50,8 @@
import java.util.concurrent.TimeUnit;
@Tags({"bulletin", "site", "site to site"})
-@CapabilityDescription("Publishes Bulletin events using the Site To Site
protocol. Note: only up to 5 bulletins are stored per component and up to "
+@CapabilityDescription("Publishes Bulletin events using the Site To Site
protocol. Bulletins can be filtered by a configurable minimum severity level. "
+ + "Note: only up to 5 bulletins are stored per component and up to "
Review Comment:
I recommend removing the additional description, it is covered under the
property description.
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -63,6 +64,15 @@
@DefaultSchedule(strategy = SchedulingStrategy.TIMER_DRIVEN, period = "1 min")
public class SiteToSiteBulletinReportingTask extends
AbstractSiteToSiteReportingTask {
+ static final PropertyDescriptor BULLETIN_LEVEL = new
PropertyDescriptor.Builder()
+ .name("Bulletin Level")
+ .description("The minimum level of bulletins to report. Bulletins
at this level and above will be "
+ + "sent. For example, selecting WARNING will send WARNING
and ERROR bulletins, but not INFO.")
Review Comment:
Multi-line strings should not use string concatenation.
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/test/java/org/apache/nifi/reporting/TestSiteToSiteBulletinReportingTask.java:
##########
@@ -166,6 +167,49 @@ public void testSerializedFormWithNullValues() throws
IOException, Initializatio
assertEquals(JsonValue.NULL, bulletinJson.get("bulletinSourceType"));
}
+ @Test
+ public void testBulletinLevelFilter() throws IOException,
InitializationException {
+ final List<Bulletin> bulletins = new ArrayList<>();
+ bulletins.add(BulletinFactory.createBulletin("group-id", "group-name",
"source-id", "source-name", "category", Severity.INFO.name(), "info message"));
+ bulletins.add(BulletinFactory.createBulletin("group-id", "group-name",
"source-id", "source-name", "category", Severity.WARNING.name(), "warning
message"));
+ bulletins.add(BulletinFactory.createBulletin("group-id", "group-name",
"source-id", "source-name", "category", Severity.ERROR.name(), "error
message"));
Review Comment:
Repeated string values should be declared statically and reused in this
method.
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -113,22 +124,30 @@ public void onTrigger(final ReportingContext context) {
final String platform =
context.getProperty(SiteToSiteUtils.PLATFORM).evaluateAttributeExpressions().getValue();
final Boolean allowNullValues =
context.getProperty(ALLOW_NULL_VALUES).asBoolean();
+ final String configuredLevel =
context.getProperty(BULLETIN_LEVEL).getValue();
+ final Severity minimumSeverity = Severity.valueOf(configuredLevel);
final Map<String, ?> config = Collections.emptyMap();
final JsonBuilderFactory factory = Json.createBuilderFactory(config);
final JsonObjectBuilder builder = factory.createObjectBuilder();
final long start = System.nanoTime();
- // Create a JSON array of all the events in the current batch
+ // Create a JSON array of bulletins that meet the minimum severity
level
final JsonArrayBuilder arrayBuilder = factory.createArrayBuilder();
for (final Bulletin bulletin : bulletins) {
- if (bulletin.getId() > lastSentBulletinId) {
+ if (bulletin.getId() > lastSentBulletinId &&
meetsMinimumSeverity(bulletin, minimumSeverity)) {
Review Comment:
I recommend combining this logic into a single method named something like,
`isBulletinIncluded`
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -113,22 +124,30 @@ public void onTrigger(final ReportingContext context) {
final String platform =
context.getProperty(SiteToSiteUtils.PLATFORM).evaluateAttributeExpressions().getValue();
final Boolean allowNullValues =
context.getProperty(ALLOW_NULL_VALUES).asBoolean();
+ final String configuredLevel =
context.getProperty(BULLETIN_LEVEL).getValue();
+ final Severity minimumSeverity = Severity.valueOf(configuredLevel);
final Map<String, ?> config = Collections.emptyMap();
final JsonBuilderFactory factory = Json.createBuilderFactory(config);
final JsonObjectBuilder builder = factory.createObjectBuilder();
final long start = System.nanoTime();
- // Create a JSON array of all the events in the current batch
+ // Create a JSON array of bulletins that meet the minimum severity
level
final JsonArrayBuilder arrayBuilder = factory.createArrayBuilder();
for (final Bulletin bulletin : bulletins) {
- if (bulletin.getId() > lastSentBulletinId) {
+ if (bulletin.getId() > lastSentBulletinId &&
meetsMinimumSeverity(bulletin, minimumSeverity)) {
arrayBuilder.add(serialize(builder, bulletin, platform,
nodeId, allowNullValues));
}
}
final JsonArray jsonArray = arrayBuilder.build();
+ if (jsonArray.isEmpty()) {
+ getLogger().debug("No bulletins to send after filtering by minimum
level [{}].", configuredLevel);
Review Comment:
The `.` should not be used at the end of log messages.
```suggestion
getLogger().debug("No bulletins to send after filtering by
minimum level [{}]", configuredLevel);
```
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -63,6 +64,15 @@
@DefaultSchedule(strategy = SchedulingStrategy.TIMER_DRIVEN, period = "1 min")
public class SiteToSiteBulletinReportingTask extends
AbstractSiteToSiteReportingTask {
+ static final PropertyDescriptor BULLETIN_LEVEL = new
PropertyDescriptor.Builder()
+ .name("Bulletin Level")
Review Comment:
Aligning with the description, recommend naming this `Minimum Bulletin
Level`.
```suggestion
static final PropertyDescriptor MINIMUM_BULLETIN_LEVEL = new
PropertyDescriptor.Builder()
.name("Minimum Bulletin Level")
```
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -170,6 +189,14 @@ public void onTrigger(final ReportingContext context) {
lastSentBulletinId = currMaxId;
}
+ private boolean meetsMinimumSeverity(final Bulletin bulletin, final
Severity minimumSeverity) {
+ try {
+ return Severity.valueOf(bulletin.getLevel()).ordinal() >=
minimumSeverity.ordinal();
Review Comment:
The severity value should be declared before performing the comparison for
better readability.
##########
nifi-extension-bundles/nifi-site-to-site-reporting-bundle/nifi-site-to-site-reporting-task/src/main/java/org/apache/nifi/reporting/SiteToSiteBulletinReportingTask.java:
##########
@@ -63,6 +64,15 @@
@DefaultSchedule(strategy = SchedulingStrategy.TIMER_DRIVEN, period = "1 min")
public class SiteToSiteBulletinReportingTask extends
AbstractSiteToSiteReportingTask {
+ static final PropertyDescriptor BULLETIN_LEVEL = new
PropertyDescriptor.Builder()
+ .name("Bulletin Level")
+ .description("The minimum level of bulletins to report. Bulletins
at this level and above will be "
+ + "sent. For example, selecting WARNING will send WARNING
and ERROR bulletins, but not INFO.")
+ .required(true)
+ .allowableValues(Severity.INFO.name(), Severity.WARNING.name(),
Severity.ERROR.name())
+ .defaultValue(Severity.WARNING.name())
Review Comment:
This should be `INFO` to align with existing behavior.
--
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]
