Created channel interface for KVEvents processing and added unit test cases #77
Conversation
praddy26
force-pushed
the
kvevents-interface-abstraction
branch
2 times, most recently
from
95e69c0 to
ea6d11e
Compare
praddy26
force-pushed
the
kvevents-interface-abstraction
branch
from
ea6d11e to
75a0934
Compare
praddy26
force-pushed
the
kvevents-interface-abstraction
branch
from
75a0934 to
966992a
Compare
There was a problem hiding this comment.
Thank you for your contribution! Overall looks good.
In general I think it is too focused on mocks and adding functionality. In my view it should be focused on testing what already exists. It would be ideal to limit it to the Channel abstraction and the testing of the existing components.
| ) | ||
|
|
||
| // mockIndex is a simple in-memory implementation of kvblock.Index for testing. | ||
| type mockIndex struct { |
There was a problem hiding this comment.
Why not use the kvblock.Index? a lot of what's here is a duplication of that. The in-memory index is lightweight enough. I think it's better if we use the index here directly.
| // Channel represents an abstract message channel for KV events. | ||
| // This interface allows for different implementations (ZMQ, HTTP SSE, NATS, test mocks, etc.) | ||
| // providing extensibility and better testability as suggested in issue #46. |
There was a problem hiding this comment.
The description should be self-contained decoupled from the issue.
| // Channel represents an abstract message channel for KV events. | ||
| // This interface allows for different implementations (ZMQ, HTTP SSE, NATS, test mocks, etc.) | ||
| // providing extensibility and better testability as suggested in issue #46. | ||
| type Channel interface { |
There was a problem hiding this comment.
I like the minimal interface but it does not bridge between the implementation and the pool. The interface should include the registration of the AddTask function (or any equivalent).
|
|
||
| // Publisher represents an abstract publisher for KV events. | ||
| // This interface allows for different publishing implementations. | ||
| type Publisher interface { |
There was a problem hiding this comment.
This is good for testing only, should be moved to the _test.go file.
| // HTTPSSEChannel implements the Channel interface using HTTP Server-Sent Events. | ||
| // This is useful for scenarios where ZMQ is not available or when HTTP-based | ||
| // communication is preferred. | ||
| type HTTPSSEChannel struct { |
There was a problem hiding this comment.
Do you see any use for this? or is it just a reference? I think at this stage it will only be the ZMQ subscriber, the NATS.io one will follow in a couple of releases. I think this can be either ejected or transformed to a "dummy-channel" or "reference-channel", otherwise it might be confusing.
This could interest the simulator folks.
| } | ||
|
|
||
| // HTTPSSEPublisher implements the Publisher interface using HTTP POST requests. | ||
| type HTTPSSEPublisher struct { |
There was a problem hiding this comment.
This should not be mixed with the channel. Publishers are only for testing in our PoV.
| Copyright 2025 The llm-d Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this f // Parse SSE format |
There was a problem hiding this comment.
This code was accidentally placed here.
|
|
||
| func TestZMQSubscriberImplementsChannel(t *testing.T) { | ||
| // This test verifies that zmqSubscriber implements the Channel interface | ||
| // We can't easily test the actual ZMQ functionality without external dependencies, |
There was a problem hiding this comment.
We do want to test ZMQ works correctly.
| return foundKeys, keyToPods, nil | ||
| } | ||
|
|
||
| func TestChannelInterfaceAbstraction(t *testing.T) { |
There was a problem hiding this comment.
I'm not sure there is much value in testing a fully mock flow (apart from testing the mocks - but they are dominating this PR). The channel interface only declares a pair of Start and Close functions - the testing here is purely on mocks. I think both should be ejected to refrain from maintaining them, plus at this stage the focus is on testing the existing code.
| // Package main demonstrates the use of different Channel implementations | ||
| // for KV Events processing, showcasing the extensibility provided by the | ||
| // Channel interface abstraction introduced in issue #46. | ||
| package main |
There was a problem hiding this comment.
Demos and examples should be in the examples directory.
No description provided.