Add service level monitoring support#1539
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds service-level monitoring support to the FTP module, enabling multiple services attached to a single listener to monitor different paths independently through the @ftp:ServiceConfig annotation. This is a significant architectural enhancement that improves flexibility and modularity.
Changes:
- Introduces
@ftp:ServiceConfigannotation for service-level path configuration - Implements multi-path routing with
MultiPathServerConnectorandPathSpecificListener - Deprecates listener-level
path,fileNamePattern,fileAgeFilter, andfileDependencyConditionsfields in favor of service-level configuration
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
native/src/main/java/io/ballerina/stdlib/ftp/util/FtpConstants.java |
Adds constants for service-level configuration annotation |
native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/PathSpecificListener.java |
New wrapper class that adds path context to events for routing |
native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java |
New connector implementation supporting multiple path consumers |
native/src/main/java/io/ballerina/stdlib/ftp/transport/message/RemoteFileSystemEvent.java |
Adds sourcePath field for routing events to correct service |
native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceConfiguration.java |
New immutable configuration class using builder pattern |
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java |
Extends registration logic to handle service-level configs, validates consistency, switches connectors |
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java |
Implements path-based routing logic and maintains service configurations |
ballerina/tests/service_config_test.bal |
Comprehensive tests for multi-path routing, single service, backward compatibility |
ballerina/listener_endpoint.bal |
Deprecates listener-level configuration fields |
ballerina/annotations.bal |
Defines ServiceConfiguration record and ServiceConfig annotation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java
Outdated
Show resolved
Hide resolved
...o/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java
Outdated
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the unresolved comments in this thread |
|
@niveathika I've opened a new pull request, #1540, to work on those changes. Once the pull request is ready, I'll request review from you. |
niveathika
added
the
Skip GraalVM Check
niveathika
force-pushed
the
service-attach
branch
2 times, most recently
from
626e1c9 to
0a033e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Show resolved
Hide resolved
...o/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Outdated
Show resolved
Hide resolved
niveathika
requested review from
DimuthuMadushan,
MohamedSabthar,
ThisaruGuruge and
shafreenAnfar
as code owners
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (77.52%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1539 +/- ##
============================================
- Coverage 77.83% 77.52% -0.31%
- Complexity 718 989 +271
============================================
Files 55 74 +19
Lines 3582 4583 +1001
Branches 599 789 +190
============================================
+ Hits 2788 3553 +765
- Misses 552 686 +134
- Partials 242 344 +102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@niveathika I've opened a new pull request, #1542, to work on those changes. Once the pull request is ready, I'll request review from you. |
niveathika
force-pushed
the
service-attach
branch
from
71280ca to
08838e9
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds service-level FTP monitoring via a new ServiceConfiguration type and Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Listener
participant FtpListenerHelper
participant MultiPathConnector
participant PathListener
participant RemoteFS
Client->>Listener: new Listener(baseConfig)
Listener->>FtpListenerHelper: init(baseConfig)
FtpListenerHelper->>FtpListenerHelper: store base params, defer connector creation
Client->>Listener: attach(service with `@ServiceConfig` path="/route-a")
Listener->>FtpListenerHelper: register(service)
FtpListenerHelper->>FtpListenerHelper: parse ServiceConfig, validate
FtpListenerHelper->>MultiPathConnector: create(baseParams, ftpListener)
FtpListenerHelper->>MultiPathConnector: addPathConsumer("/route-a", patterns, filters)
MultiPathConnector->>PathListener: create(listener, "/route-a")
MultiPathConnector->>RemoteFS: create consumer for /route-a
RemoteFS->>MultiPathConnector: poll()
MultiPathConnector->>PathListener: notify consumer
PathListener->>PathListener: setSourcePath("/route-a")
PathListener->>Listener: onMessage(event with sourcePath)
Listener->>Listener: route event to service bound to "/route-a"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ballerina/tests/client_endpoint_test.bal (1)
1203-1249:⚠️ Potential issue | 🟠 MajorFix resourceNames/fileSizes length mismatch (and duplicate entry).
resourceNameshas 23 entries whilefileSizeshas 22, sofileSizes[i]will throw out-of-bounds whenireaches the extra entry. There’s also a duplicate"onerror-tests"which likely caused the mismatch.If the duplicate is intentional, add the missing size entry instead.🛠️ Possible fix (remove the duplicate entry)
- "test3.txt", - "onerror-tests", + "test3.txt", "sc-route-a",
🤖 Fix all issues with AI agents
In `@ballerina/tests/service_config_test.bal`:
- Around line 107-155: The testMultiPathRouting function uses module-level state
(routeAEventCount, routeBEventCount, routeALastFileName, routeBLastFileName)
which can leak between tests; reset those variables at the start of
testMultiPathRouting by setting routeAEventCount = 0, routeBEventCount = 0 and
clearing routeALastFileName/routeBLastFileName (empty string or nil as
appropriate) before creating the Listener and attaching services so each run
starts with clean state.
In `@changelog.md`:
- Line 10: Update the changelog entry text "- [Add service level monitoring
config](https://github.com/ballerina-platform/ballerina-library/issues/8606)" to
hyphenate "service-level" (i.e., change "service level monitoring config" to
"service-level monitoring config") so the markdown entry reads "- [Add
service-level monitoring
config](https://github.com/ballerina-platform/ballerina-library/issues/8606)".
In `@native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java`:
- Around line 574-578: The cleanup method currently clears registeredServices,
serviceConfigurations, pathToService and resets caller but leaves the
usesServiceLevelConfig flag set; update the cleanup() method in FtpListener to
also reset the usesServiceLevelConfig field (set it back to false) so that when
the listener is reused legacy services are not incorrectly rejected; locate the
usesServiceLevelConfig variable in the FtpListener class and ensure cleanup()
assigns it false alongside the other clears.
- Around line 518-526: The addServiceConfiguration method currently uses
containsKey + put on pathToService which is racy; replace that logic with an
atomic putIfAbsent on pathToService (call pathToService.putIfAbsent(path,
service) and check the return value) and only when putIfAbsent returns null
(meaning we successfully inserted) should you add the entry into
serviceConfigurations and set usesServiceLevelConfig.set(true); if putIfAbsent
indicates a preexisting entry, return false without modifying
serviceConfigurations or usesServiceLevelConfig.
In `@native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java`:
- Around line 134-165: Defer registering the service-level config so stale
mappings aren't left if connector creation fails: move the call to
listener.addServiceConfiguration(service, config) (and any listener flag like
usesServiceLevelConfig) to after createMultiPathConnector(...) has successfully
completed and ftpConnector is obtained, then proceed with casting to
MultiPathServerConnector and calling multiPathConnector.addPathConsumer(...),
listener.setFileSystemManager(...), and setFileSystemOptions(...);
alternatively, if you must call addServiceConfiguration before
createMultiPathConnector, wrap createMultiPathConnector in a try/catch and on
any failure remove the previously added config (e.g.,
listener.removeServiceConfiguration or equivalent) so the listener state is not
left with stale mappings.
In
`@native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceConfiguration.java`:
- Around line 37-113: Ensure ServiceConfiguration enforces a non-null required
path and preserves immutability of dependencyConditions: in Builder.build()
(and/or the ServiceConfiguration(Builder) ctor) validate that Builder.path is
not null/empty and throw an IllegalStateException with a clear message if
missing, and defensively copy the dependencyConditions (e.g., use
List.copyOf(...) or Collections.unmodifiableList(new ArrayList<>(...))) instead
of assigning the builder list reference so the internal
List<FileDependencyCondition> cannot be mutated by callers; update assignments
in ServiceConfiguration(Builder) and/or Builder.dependencyConditions to perform
the immutable copy.
🧹 Nitpick comments (3)
docs/spec/spec.md (1)
1249-1249: Minor: Use hyphenated compound adjective."high availability" should be hyphenated when used as a compound adjective modifying "deployments".
📝 Suggested fix
-The FTP listener supports distributed coordination for high availability deployments. +The FTP listener supports distributed coordination for high-availability deployments.native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java (1)
557-563: Javadoc key mismatch (path vs service name).
The map is keyed by path, but the comment says “service name.” Consider aligning the Javadoc to avoid confusion.native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java (1)
351-369: Prefer exact annotation-key matching to avoid false positives.
UsingendsWithcan match unrelated annotations from other modules with the same suffix.
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java
Show resolved
Hide resolved
native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceConfiguration.java
Show resolved
Hide resolved
niveathika
force-pushed
the
service-attach
branch
from
08838e9 to
56d9f1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ballerina/tests/service_config_test.bal`:
- Around line 479-505: The test creates a Listener (ftpListener) but never stops
it, causing resource leaks if assertions fail; update
testServiceConfigInvalidFileNamePattern to ensure ftpListener is cleaned up by
calling ftpListener.stop() (or the listener's proper shutdown method) in a
finally block or a try/finally around the attach/assert logic, and apply the
same cleanup pattern to testServiceConfigInvalidTargetPattern and
testServiceConfigInvalidRequiredFilesPattern so the listener is always stopped
regardless of test outcome.
🧹 Nitpick comments (4)
native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java (1)
141-143: Consider usinglog.infoinstead oflog.warnfor expected behavior.This log message describes normal, expected behavior when a service uses
@ftp:ServiceConfig. Usingwarnlevel suggests something is wrong, but this is simply informational about which configuration takes precedence.♻️ Suggested change
- log.warn("Creating multi-path connector for service with `@ftp`:ServiceConfig annotation " + + log.info("Creating multi-path connector for service with `@ftp`:ServiceConfig annotation " + "and path '{}'. The listener-level 'path' configuration will be ignored.", config.getPath());native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java (3)
49-51: Consider makingisPollOperationOccupiedfinal.The
AtomicBooleanis never reassigned, so it should befinalfor clarity and to prevent accidental reassignment.♻️ Suggested fix
- private AtomicBoolean isPollOperationOccupied = new AtomicBoolean(false); + private final AtomicBoolean isPollOperationOccupied = new AtomicBoolean(false);
94-98: Potential race condition when initializing shared FileSystemManager.If
addPathConsumeris called concurrently from multiple threads, both could pass thefileSystemManager == nullcheck and overwrite the field. While the effect is likely benign (same manager type), this is a subtle thread-safety issue.Consider synchronizing initialization or using a more defensive pattern if concurrent registration is expected.
♻️ Suggested fix using synchronized block
pathConsumers.put(path, consumer); // Store FileSystemManager and options from first consumer - if (fileSystemManager == null) { - fileSystemManager = consumer.getFileSystemManager(); - fileSystemOptions = consumer.getFileSystemOptions(); + synchronized (this) { + if (fileSystemManager == null) { + fileSystemManager = consumer.getFileSystemManager(); + fileSystemOptions = consumer.getFileSystemOptions(); + } }Is
addPathConsumercalled concurrently during service attachment, or is it guaranteed to be single-threaded during listener startup?
194-209: Consider logging all errors, not just returning the last one.The
stop()method returns only the last error encountered, meaning earlier errors are silently lost. While the interface may constrain the return type, logging all errors would aid debugging.♻️ Suggested enhancement
`@Override` public Object stop() { Object lastError = null; for (Map.Entry<String, RemoteFileSystemConsumer> entry : pathConsumers.entrySet()) { try { Object error = entry.getValue().close(); if (error != null) { + log.warn("Error closing consumer for path {}: {}", entry.getKey(), error); lastError = error; } } catch (Exception e) { log.error("Error stopping consumer for path: " + entry.getKey(), e); } } pathConsumers.clear(); return lastError; }
Docstrings generation was requested by @niveathika. * #1539 (comment) The following files were modified: * `native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java` * `native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListenerHelper.java` * `native/src/main/java/io/ballerina/stdlib/ftp/server/ServiceConfiguration.java` * `native/src/main/java/io/ballerina/stdlib/ftp/transport/RemoteFileSystemConnectorFactory.java` * `native/src/main/java/io/ballerina/stdlib/ftp/transport/impl/RemoteFileSystemConnectorFactoryImpl.java` * `native/src/main/java/io/ballerina/stdlib/ftp/transport/message/RemoteFileSystemEvent.java` * `native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java` * `native/src/main/java/io/ballerina/stdlib/ftp/transport/server/connector/contractimpl/PathSpecificListener.java` * `test-utils/src/main/java/io/ballerina/stdlib/ftp/testutils/mockServerUtils/MockFtpServer.java`
niveathika
force-pushed
the
service-attach
branch
from
56d9f1d to
a70facd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/spec/spec.md`:
- Around line 919-920: The sentence in the spec misleadingly states that
ftp:ListenerConfiguration requires a listener-level path; update it to say that
ftp:ListenerConfiguration requires protocol and host, and that the path should
be supplied via the `@ftp`:ServiceConfig annotation (listener-level path fields
are deprecated). Reference ftp:ListenerConfiguration, the deprecated
listener-level path field, and `@ftp`:ServiceConfig in the revised sentence.
- Around line 879-881: The spec currently states that `@ftp`:ServiceConfig.path
"must be an absolute path starting with '/'", but the runtime accepts and
normalizes relative paths; update the table row for "Invalid path pattern" to
reflect that relative paths are accepted and normalized (or remove the hard
requirement). Specifically, change the error message string for the "Invalid
path pattern" entry (the cell currently containing "Invalid path '{path}' in
`@ftp`:ServiceConfig. Path must be an absolute path starting with '/'." ) to
indicate that paths may be relative and will be normalized (e.g., "Invalid path
'{path}' in `@ftp`:ServiceConfig. Paths may be relative; runtime normalizes them
to an absolute path starting with '/'.") or otherwise document the automatic
normalization behavior for `@ftp`:ServiceConfig.path.
🧹 Nitpick comments (2)
ballerina/tests/secure_ftps_listener_endpoint_test.bal (1)
243-276: Consider consistency in error verification patterns across tests.Some tests use
Listener|Errorwith constructor-time error checks (e.g.,testFtpsConnectToFTPServerWithFTPProtocolat line 319), while others usecheck newwith attach-based error verification. This inconsistency may be intentional if certain errors are expected at construction time vs. attach time, but documenting this distinction would improve maintainability.The attach-based pattern with inline
attachServicedefinitions works correctly for the intended test scenarios.native/src/main/java/io/ballerina/stdlib/ftp/server/FtpListener.java (1)
124-135: Log when a source path has no registered service.
WhenusesServiceLevelConfigis enabled and no service matchesevent.getSourcePath(), the event is dropped silently. A warning would make misconfigurations far easier to debug.💡 Suggested tweak
if (usesServiceLevelConfig.get() && event.getSourcePath() != null) { // Path-keyed routing: dispatch only to the service monitoring this path BObject service = pathToService.get(event.getSourcePath()); if (service != null) { dispatchFileEventToService(this.environment, service, event); + } else { + log.warn("No service registered for source path: {}", event.getSourcePath()); } } else {
...o/ballerina/stdlib/ftp/transport/server/connector/contractimpl/MultiPathServerConnector.java
Show resolved
Hide resolved
niveathika
force-pushed
the
service-attach
branch
from
6c6a8b4 to
c55a1c2
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ballerina/tests/secure_ftps_listener_endpoint_test.bal`:
- Around line 306-309: The assertion is checking for "Failed to load KeyStore
from path" while the test is validating an invalid truststore, which is
confusing; update the test assertion or add a clarifying comment near the
ftpsServer.attach call and the test:assertTrue usage to explicitly state that
KeyStore is the Java class used for both keystore and truststore, or change the
test assertion message to mention "truststore" (e.g., include "truststore
(KeyStore) failed to load from path") so the failure message and the test name
align with attachResult, ftpsServer.attach, and test:assertTrue.
🧹 Nitpick comments (1)
ballerina/tests/secure_ftps_listener_endpoint_test.bal (1)
265-268: Extract duplicateattachServiceto a shared constant.The identical
attachServicedefinition is repeated 6 times across the negative test cases (lines 265-268, 302-305, 360-363, 396-399, 472-475, 513-516). Consider extracting this to a file-level constant to reduce duplication and simplify maintenance.♻️ Suggested refactor
Add a shared constant at the top of the file (e.g., near line 28):
// Minimal service for error-path testing final Service noOpAttachService = service object { remote function onFileChange(WatchEvent & readonly event) { } };Then replace all inline
attachServicedefinitions with:- Service attachService = service object { - remote function onFileChange(WatchEvent & readonly event) { - } - }; - error? attachResult = ftpsServer.attach(attachService); + error? attachResult = ftpsServer.attach(noOpAttachService);
Purpose
Fixes ballerina-platform/ballerina-library#8606
Examples
Checklist
Summary by CodeRabbit
New Features
Deprecations
Documentation
Tests