DLQ API for unsideline & GetMessages #168

kmrdhruv · 2024-10-22T05:00:22Z

Changes for DLQ APIs.

Unsideline requests
GetMessages
Support for multi permission check in route behavior.

TODO:
Tests to be added post initial review.

- message channel PR comments

…luster manager

…ution and updates on subscription operation for a subscription

Works for dev setup for metric collection. For prod additional changes will be needed. - Convert the otel container as a sidecar - Test traces and logs in addition to metrics. - Promethous configuration for scrapping needs to be defined.

updating job definition due to container rename.

- Unsideline & GetMessages - Changes to support multi permission checks in RouteBehavior

github-actions · 2024-10-24T18:04:27Z

Test Results

280 tests 280 ✅ 54s ⏱️
55 suites 0 💤
55 files 0 ❌

Results for commit 7468d98.

gauravAshok · 2024-10-30T16:13:25Z

we should start evaluating java 21.
most of the control flow will be simplified if we do that.

gauravAshok · 2024-10-30T16:38:25Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesRequest.java

+import java.util.List;
+
+@Data
+public class GetMessagesRequest {


I think we should put all entities specific to web api in a web package. preferably versioned? as in "web.v1."?

aligned, will take independent of this PR. There are other entities as well, which would need to be moved.

gauravAshok · 2024-10-30T16:40:37Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesResponse.java

+
+@Data
+public class GetMessagesResponse {
+    private int shardId;


why should shardId be part of response?

gauravAshok · 2024-10-30T16:48:01Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesRequest.java

+    public static final long UNSPECIFIED_TS = 0L;
+    public static final long LATEST_TS = Long.MAX_VALUE;
+
+    //TODO:: evaluate if earliestFailedAt should be per shard level to support pagination kind of semantics.


i am hoping that shard semantics, is totally transparent and is not leaking into public api. Shard is just "a" implementation of scaling subscription, but not the only one. sharding strategies may also differ. so an operation suitable for one shard strategy might not work for another shard strategy.

So, I was hoping that most operations are agnostic of "shards", if any operation needs to be mapped then it is done either by "server" or "controller". most likely controller as that guy has the know how.

gauravAshok · 2024-10-30T16:48:39Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesRequest.java

+
+    //TODO:: evaluate if earliestFailedAt should be per shard level to support pagination kind of semantics.
+    private long earliestFailedAt = 0L;
+    private List<Offset> offsets = new ArrayList<>();


What does list of offsets represent here?

Got it. It looks it is for paging. But as discussed, it can't just be a pulsar offset. we also need the shard info embedded in it.

gauravAshok · 2024-10-30T16:50:25Z

core/src/main/java/com/flipkart/varadhi/core/cluster/ConsumerApi.java

 import java.util.concurrent.CompletableFuture;

 public interface ConsumerApi {
    CompletableFuture<Void> start(ShardOperation.StartData operation);

    CompletableFuture<Void> stop(ShardOperation.StopData operation);

+    CompletableFuture<Void> unsideline(ShardOperation.UnsidelineData operation);


what does Future<> as the return value mean here? for unsideline op?
Given that the unsideline operation may take extended amount of time.

gauravAshok · 2024-11-05T14:22:37Z

I reviewed it largely.

I haven't check all corner cases, as I am still going through the overall architecture and how all classes are placed and interacting. Post that I will be able to comment on the corner cases.

There are common patterns in controller that I feel can be simplified a lot. (with proper abstraction). Instead of each opExecutor handling its corner cases, I think all the complications can be abstracted away. I will think about this.

gauravAshok · 2024-10-30T16:58:09Z

entities/src/main/java/com/flipkart/varadhi/entities/cluster/UnsidelineOpRequest.java

+
+@Data
+@AllArgsConstructor
+public class UnsidelineOpRequest {


These cluster related entities does not look they belong in entities module.
They are internal to varadhi's protocol and should probably exist elsewhere.

I was hoping entities module be just outside world facing pojos, nothing else.

in that case we would need to split the entities for internal vs external.

maybe that is for the better. why couple them?

gauravAshok · 2024-11-05T08:33:36Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesRequest.java

+
+    //TODO:: evaluate if earliestFailedAt should be per shard level to support pagination kind of semantics.
+    private long earliestFailedAt = 0L;
+    private List<Offset> offsets = new ArrayList<>();


Got it. It looks it is for paging. But as discussed, it can't just be a pulsar offset. we also need the shard info embedded in it.

gauravAshok · 2024-11-05T08:35:36Z

entities/src/main/java/com/flipkart/varadhi/entities/GetMessagesResponse.java

+public class GetMessagesResponse {
+    private int shardId;
+    private List<Message> messages;
+    private String error;


error is part of the payload because of the partial failure scenario? failures from only some shards!

gauravAshok · 2024-11-05T10:12:37Z

entities/src/main/java/com/flipkart/varadhi/entities/cluster/ShardOperation.java

@@ -51,6 +51,12 @@ public static ShardOperation stopOp(String subOpId, SubscriptionUnitShard shard,
        return new ShardOperation(new ShardOperation.StopData(subOpId, shard, subscription));
    }

+    public static ShardOperation unsidelineOp(
+            String subOpId, SubscriptionUnitShard shard, VaradhiSubscription subscription, UnsidelineRequest request


any reason we are putting the whole object (shard info, subscription info) in the operation object.

Any particular usecase does it serve? What if we just put ids in instead? Like subscription identifier, shard id.

Consumer nodes doesn't interact with metastore, so all information is passed via message alone.

But, the unsideline operation is alreayd happening within a context of a running subscription in a consumer. the unsideline can only happen when the consumer is running that subscription and knows about it. Hence I wondered by unsidelineOp needs the full object. StartOp containing full object makes sense. Similarly I wont expect stopOp to contain full object.

oh..that way, makes sense. I will take it independent in next PR (along with entities refactoring).

gauravAshok · 2024-11-05T10:17:32Z

entities/src/testFixtures/java/com/flipkart/varadhi/entities/SubProvider.java

@@ -64,6 +64,7 @@ public static class Builder {
        private RetryPolicy retryPolicy;
        private ConsumptionPolicy consumptionPolicy;
        private SubscriptionShards shards;
+        private Map<String, String> properties = new HashMap<>();


what is subProvider?

test helper related to subscription entities. renamed as SubscriptionUtil

gauravAshok · 2024-11-05T13:54:59Z

server/src/main/java/com/flipkart/varadhi/services/DlqService.java

+                                    shardResponse.setError(t.getMessage());
+                                }
+                                log.info("Sending a GetMessage response for {}", assignment);
+                                responseWriter.accept(shardResponse);


This does not look correct.
consumer.getMessages() will run in parallel, so we may be doing responseWriter(shardResponse) in parallel as well, where responseWrite is being shared among these "threads". is this expected to work?

yeah same thought as well, merging across steams without well defined boundary will not work. Currently responses from shards is not chuncked, but a single response.

gauravAshok · 2024-11-05T14:01:28Z

server/src/main/java/com/flipkart/varadhi/services/SubscriptionService.java

@@ -14,13 +14,13 @@
 @Slf4j
 public class SubscriptionService {
    private final MetaStore metaStore;
-    private final ControllerApi controllerApi;
+    private final ControllerApi controllerClient;


NIT: in general we can skip using the word client. client enforces the idea of it being remote, even though that is not a hard requirement.

gauravAshok · 2024-11-05T14:02:39Z

server/src/main/java/com/flipkart/varadhi/utils/AsyncProducerReadStream.java

+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class AsyncProducerReadStream implements ReadStream<Buffer> {


is there a resource which you followed for this?

gauravAshok · 2024-11-05T14:07:08Z

setup/docker/compose.yml

@@ -40,7 +41,8 @@ services:

  pulsar:
    image: apachepulsar/pulsar:3.0.0
-    profiles: [ "test", "dev", "pulsar" ]
+    container_name: pulsar
+    profiles: [ "test", "dev", "pulsar" , "dev-metric"]


i think we can include the prometheus into the test and dev profile by default. I dont think metric specific profile makes sense.

possibly yes, might need to tune the resources here otherwise laptop becomes issue. Will see if this can be done (might keep same for now).

controller/src/main/java/com/flipkart/varadhi/controller/impl/opexecutors/StartOpExecutor.java

kmrdhruv added 30 commits February 13, 2024 23:12

messagechannel.send() implementation

f571f2c

Merge branch 'master' into controller

fc704e9

moving cluster related details to server module

b021412

merge

edcd600

Merge branch 'master' into controller

59cc57a

- refactoring needed for controller bootstrapping

f5d484c

- message channel PR comments

Merge branch 'master' into controller

f243139

add string override to channel method

f498775

fix commented test and remove obselete message handler

c0d9a5e

Merge branch 'master' into controller

0c7b269

review comments

1fd93f8

avoid http port conflict in for server deployment in UTs

1f43d2e

Merge branch 'master' into controller

597df03

cluster manager and refactoring

ccfed68

fixing TODOs, Router tests and implementation of getAllMembers() in c…

249d957

…luster manager

Merge branch 'master' into controller

8d849a3

Merge branch 'master' into controller

48d28b2

replacing Component with Verticle

4caa516

removing hard coded 127.0.0.1

08ad83f

class name fix in gradle file

a97b531

Merge branch 'master' into controller

61b1175

StartSubscription operation and Consumer verticle bootsrap code flow.

615c422

Merge branch 'master' into controller

f978900

adding persistence to shard assignment and operations.

9fa7a9f

Merge branch 'master' into controller

50c96b1

pr fixes and improvements

19a1363

fixing subscription failing test

68bc1b3

Merge branch 'master' into controller

caf9082

Merge branch 'master' into controller

094a279

serializing the shard assignment for all, subscription operation exec…

16211de

…ution and updates on subscription operation for a subscription

kmrdhruv added 17 commits June 24, 2024 16:00

fixes and refactoring

7d88e6a

controller restart handling initial implementation.

ce25db4

Merge branch 'master' into controller

663014f

Tests for controller module.

51e13fc

Merge branch 'master' into controller

31f0dd9

refactoring related to version handling in Versioned entities.

b415bf5

retry for failed operation

c913da6

Merge branch 'master' into controller

640c825

retry for failed operations -- bug fixes.

61b253f

test cases for retry

6bba83b

Merge branch 'master' into controller

6f831b4

cr comments

c3858ff

reverting stale changes to test code which got pushed accidently.

f42587b

Merge branch 'master' into controller

5ff9fcd

Update e2e.yml

c8510b4

updating job definition due to container rename.

Changes for DLQ API

cb49a10

- Unsideline & GetMessages - Changes to support multi permission checks in RouteBehavior

kmrdhruv requested review from gauravAshok and iosdev747 October 22, 2024 05:00

anuj-flipkart linked an issue Oct 24, 2024 that may be closed by this pull request

DLQ API for unsideline & GetMessages #170

Open

Fix for failing UTs.

7468d98

gauravAshok reviewed Oct 30, 2024

View reviewed changes

gauravAshok requested changes Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DLQ API for unsideline & GetMessages #168

DLQ API for unsideline & GetMessages #168

kmrdhruv commented Oct 22, 2024

github-actions bot commented Oct 24, 2024

gauravAshok commented Oct 30, 2024

gauravAshok Oct 30, 2024

kmrdhruv Nov 6, 2024

gauravAshok Oct 30, 2024

gauravAshok Oct 30, 2024

gauravAshok Oct 30, 2024

gauravAshok Nov 5, 2024

gauravAshok Oct 30, 2024

gauravAshok commented Nov 5, 2024

gauravAshok Oct 30, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 6, 2024

gauravAshok Nov 5, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 6, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 5, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

gauravAshok Nov 5, 2024

kmrdhruv Nov 6, 2024

DLQ API for unsideline & GetMessages #168

Are you sure you want to change the base?

DLQ API for unsideline & GetMessages #168

Conversation

kmrdhruv commented Oct 22, 2024

github-actions bot commented Oct 24, 2024

Test Results

gauravAshok commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravAshok commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment