Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions connectors/rocketmq-connect-metrics-exporter/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?xml version="1.0" encoding="UTF-8"?>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test files are included in this PR. There is no unit or integration test coverage for the connector, task, or SPI loading logic — critical paths like put(), start()/stop() lifecycle, and validate() are completely untested.

<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pom.xml has no parent POM and uses a standalone hardcoded version '1.0-SNAPSHOT' with a standalone groupId. Other connector modules in this repo likely inherit from a parent POM for consistent dependency management. This may cause version drift and build inconsistency.

<groupId>org.apache.rocketmq</groupId>
<artifactId>rocketmq-connect-metrics-exporter</artifactId>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module defines its own <groupId> and <version> instead of inheriting from the parent POM (<parent> block is absent). This means it won't be built as part of the reactor, won't inherit dependency management, and won't receive the project-wide license/enforcer plugins. It will be an orphaned module that must be built and released independently.

<version>1.0-SNAPSHOT</version>
<name>rocketmq-connect-metrics-exporter</name>


<properties>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<openmessaging.connector.version>0.1.3</openmessaging.connector.version>
</properties>

<dependencies>
<dependency>
<groupId>io.openmessaging</groupId>
<artifactId>openmessaging-connector</artifactId>
<version>${openmessaging.connector.version}</version>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.6.1</version>
<configuration>
<source>${maven.compiler.source}</source>
<target>${maven.compiler.target}</target>
<compilerVersion>${maven.compiler.source}</compilerVersion>
<showDeprecation>true</showDeprecation>
<showWarnings>true</showWarnings>
</configuration>
</plugin>
</plugins>
</build>


</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package org.apache.rocket.connect.metrics.export.sink.connector;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test changes detected alongside source modifications. Consider adding tests to cover the changes.


import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.component.task.Task;
import io.openmessaging.connector.api.component.task.sink.SinkConnector;
import java.util.ArrayList;
import java.util.List;
import org.apache.rocket.connect.metrics.export.sink.util.ServiceProvicerUtil;

public class MetricsExportSinkConnector extends SinkConnector {
private KeyValue config;

private List<MetricsExporter> metricsExporters;
{
metricsExporters = ServiceProvicerUtil.getMetricsExporterServices();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instance initializer block runs at construction time, before start(). Consider moving ServiceLoader initialization to start() to avoid potential classpath issues.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The metricsExporters field is populated in an instance-initializer block, meaning ServiceLoader runs at construction time — before start() or validate(). If the connector is constructed but never started (e.g., validation fails), the SPI scan is wasted work. Consider lazy initialization in start() for consistency with the connector lifecycle.

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metricsExporters is loaded in an instance initializer block but never closed/stopped in stop(). If exporters hold resources (e.g., HTTP clients, thread pools for Prometheus scraping), they will leak because the connector's stop() only nulls out config. Consider iterating metricsExporters in stop() and calling stop() on each, or documenting that lifecycle is fully managed by the task.


@Override public List<KeyValue> taskConfigs(int maxTasks) {
List<KeyValue> configs = new ArrayList<>();
configs.add(config);
return configs;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taskConfigs(int maxTasks) always returns a single-element list regardless of maxTasks. This caps the connector to exactly one task even when the runtime requests more. For a metrics exporter this may be intentional, but it should be documented or explicitly guarded — silently ignoring maxTasks can confuse operators who configure a higher parallelism.

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taskConfigs(int maxTasks) always returns a single-element list regardless of maxTasks. If the framework distributes work across multiple tasks for parallelism, this silently caps it at one. Either honor maxTasks or explicitly document that this connector supports only a single task.


@Override public Class<? extends Task> taskClass() {
return MetricsExportSinkTask.class;
}

@Override public void start(KeyValue config) {
this.config = config;
}

@Override public void stop() {
this.config = null;
}

@Override public void validate(KeyValue config) {
for (MetricsExporter exporter: metricsExporters) {
exporter.validate(config);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package org.apache.rocket.connect.metrics.export.sink.connector;

import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.component.task.sink.SinkTask;
import io.openmessaging.connector.api.component.task.sink.SinkTaskContext;
import io.openmessaging.connector.api.data.ConnectRecord;
import io.openmessaging.connector.api.errors.ConnectException;
import java.util.List;
import org.apache.rocket.connect.metrics.export.sink.util.ServiceProvicerUtil;


public class MetricsExportSinkTask extends SinkTask {
private List<MetricsExporter> metricsExporters;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has the implementation of MetricsExporter been submitted yet?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry not yet :( . But I am trying my best to finish a exporter for Prometheus which gets the metrics from the log files, and some preliminary knowledge is got. And I hear the sftp source connector (my upstream task) is designing in the last weekly meeting. So I'm waiting for the implementation ConnectRecord in it in order to complete my parsing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metricsExporters is not initialized at declaration — it is only assigned inside init(). If the framework ever calls start() or put() before init(), this will throw a NullPointerException. In MetricsExportSinkConnector, the equivalent field is initialized via an instance-initializer block. The task should do the same, or at minimum initialize at declaration: private List<MetricsExporter> metricsExporters = ServiceProvicerUtil.getMetricsExporterServices();


@Override public void put(List<ConnectRecord> sinkRecords) throws ConnectException {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error handling in put(). If any single MetricsExporter.export() throws an exception, the remaining exporters in the list are skipped and the entire batch is lost. Consider catching per-exporter exceptions so one faulty exporter does not block the others, and log or route failures appropriately.

for (MetricsExporter exporter : metricsExporters) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metricsExporters is initialized in init() but used in put() without null check. If put() is somehow called before init(), this will NPE. Consider adding a null guard or lazy initialization.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In put(), if one exporter throws an exception, all subsequent exporters in the loop are skipped and the entire batch is lost. For a multi-exporter setup this means one faulty exporter blocks all others. Consider catching per-exporter, logging, and continuing — or aggregating exceptions before rethrowing.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If metricsExporters is empty (which it will be given the broken SPI file), put() silently discards all incoming ConnectRecords with no log or error. A warning or exception when the list is empty during start() or init() would prevent silent data loss.

exporter.export(sinkRecords);
}
}

@Override public void start(KeyValue config) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start() does not call super.start(config). While the current SinkTask base may not require it, omitting the super call can break lifecycle contracts in future framework versions. Same applies to stop().

for (MetricsExporter exporter : metricsExporters) {
exporter.start(config);
}
}

@Override public void stop() {
for (MetricsExporter exporter : metricsExporters) {
exporter.stop();
}
}

@Override public void init(SinkTaskContext sinkTaskContext) {
super.init(sinkTaskContext);
metricsExporters = ServiceProvicerUtil.getMetricsExporterServices();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.apache.rocket.connect.metrics.export.sink.connector;

import io.openmessaging.KeyValue;
import io.openmessaging.connector.api.data.ConnectRecord;
import java.util.List;

public interface MetricsExporter {
void export(List<ConnectRecord> sinkRecords);

void validate(KeyValue config);

void start(KeyValue config);

void stop();

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.apache.rocket.connect.metrics.export.sink.util;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;
import java.util.ServiceLoader;
import org.apache.rocket.connect.metrics.export.sink.connector.MetricsExporter;

/**
* @author: ming
*/

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in class name: ServiceProvicerUtil should be ServiceProviderUtil (missing d in Provider).

public class ServiceProvicerUtil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class name contains a typo: ServiceProvicerUtil should be ServiceProviderUtil. This typo propagates to every call site (connector and task classes) and will be a permanent API blemish if not fixed before merge.


Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class name 'ServiceProvicerUtil' contains a typo — should be 'ServiceProviderUtil'. This is a public API surface; fixing it later would be a breaking change.

public static List<MetricsExporter> getMetricsExporterServices(){
List<MetricsExporter> metricsExporterList = new ArrayList<>();
ServiceLoader<MetricsExporter> metricsExporters = ServiceLoader.load(MetricsExporter.class);
Iterator<MetricsExporter> iterator = metricsExporters.iterator();
while (iterator.hasNext()){
metricsExporterList.add(iterator.next());
}
return metricsExporterList;
}
}