Feature/550 queue splitter #557

gcastaldi · 2024-02-06T09:29:26Z

No description provided.

codecov · 2024-02-06T09:38:42Z

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (0483480) 47.42% compared to head (c6597cd) 48.04%.
Report is 14 commits behind head on develop.

Files	Patch %	Lines
...e/queuing/splitter/QueueSplitterConfiguration.java	54.16%	10 Missing and 1 partial ⚠️
...isspush/gateleen/queue/queuing/QueuingHandler.java	0.00%	8 Missing ⚠️
...ing/splitter/QueueSplitterConfigurationParser.java	90.90%	3 Missing ⚠️
...leen/queue/queuing/splitter/QueueSplitterImpl.java	94.11%	0 Missing and 2 partials ⚠️
...itter/executors/QueueSplitExecutorFromRequest.java	93.75%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #557      +/-   ##
=============================================
+ Coverage      47.42%   48.04%   +0.61%     
- Complexity      1790     1850      +60     
=============================================
  Files            225      232       +7     
  Lines          11820    11950     +130     
  Branches        1239     1260      +21     
=============================================
+ Hits            5606     5741     +135     
+ Misses          5720     5713       -7     
- Partials         494      496       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mcweba · 2024-02-06T13:23:46Z

...een-queue/src/main/java/org/swisspush/gateleen/queue/queuing/splitter/QueueSplitterImpl.java

+    @Override
+    public void resourceRemoved(String resourceUri) {
+        if (configResourceUri() != null && configResourceUri().equals(resourceUri)) {
+            log.info("Queue splitter configuration resource {} was removed. Going to close all kafka producers", resourceUri);


Change wrong log statement (kafka producers).

mcweba · 2024-02-06T13:24:06Z

...een-queue/src/main/java/org/swisspush/gateleen/queue/queuing/splitter/QueueSplitterImpl.java

+    public void resourceRemoved(String resourceUri) {
+        if (configResourceUri() != null && configResourceUri().equals(resourceUri)) {
+            log.info("Queue splitter configuration resource {} was removed. Going to close all kafka producers", resourceUri);
+            System.out.println("Queue splitter configuration resource removed");


Remove System.out.println

mcweba · 2024-02-06T13:30:13Z

gateleen-queue/src/main/resources/gateleen_queue_splitter_configuration_schema

@@ -0,0 +1,32 @@
+{


Do not allow other properties to be added. Use additionalProperties=false

mcweba · 2024-02-06T13:30:42Z

gateleen-queue/src/main/resources/gateleen_queue_splitter_configuration_schema

@@ -0,0 +1,32 @@
+{
+  "$schema": "http://json-schema.org/draft-04/schema#",


Add a description property to all the properties

mcweba · 2024-02-06T13:34:56Z

gateleen-queue/src/main/resources/gateleen_queue_splitter_configuration_schema

+  "additionalProperties": {
+    "$ref": "#/definitions/QueueSplitter"
+  },
+  "definitions": {


Try to add more schema constraints like oneOf to clearify that only postfixFromStatic or (postfixFromHeader or postfixFromUrl) can be used. Checkout the schema defined for gateleen-delegates where this is used.

mcweba · 2024-02-06T13:37:14Z

...queue/src/test/java/org/swisspush/gateleen/queue/queuing/splitter/QueueSplitterImplTest.java

+        queueSplitter.initialize().onComplete(event -> {
+
+            // Then
+            context.assertFalse(queueSplitter.isInitialized());


Shouldn't the queueSplitter be initialized now?

If the configuration is missing or invalid the QueueSplitterIml is not initialized

mcweba · 2024-02-06T13:46:12Z

gateleen-runconfig/src/main/java/org/swisspush/gateleen/runconfig/RunConfig.java

+                                redisProvider,
+                                request,
+                                monitoringHandler,
+                                queueSplitter != null ? queueSplitter : new NoOpQueueSplitter()));


You already do this logic in the QueuingHandler constructor. I would suggest to just use the queueSplitter even if null and do this check in the Constructor. When null is passed you could create a NoOpQueueSplitter

mcweba · 2024-02-07T13:17:09Z

...ain/java/org/swisspush/gateleen/queue/queuing/splitter/QueueSplitterConfigurationParser.java

+                    JsonObject postfixFromRequest = queueConfig.getJsonObject(POSTFIX_FROM_REQUEST_KEY);
+                    String postfixFromHeader = postfixFromRequest != null ? postfixFromRequest.getString(POSTFIX_FROM_HEADER_KEY) : null;
+                    String postfixFromUrl = postfixFromRequest != null ? postfixFromRequest.getString(POSTFIX_FROM_URL_KEY) : null;
+                    if (postfixFromRequest != null && (postfixFromHeader != null || postfixFromUrl != null)) {


postfixFromRequest != null can be omitted since you already check postfixFromHeader and postfixFromUrl

mcweba · 2024-02-07T13:19:47Z

gateleen-queue/src/main/resources/gateleen_queue_splitter_configuration_schema

@@ -1,5 +1,6 @@
 {
  "$schema": "http://json-schema.org/draft-04/schema#",
+  "description": "A list of queue splitter configurations",


queue splitter configurations are not in a list ;-)

mcweba · 2024-02-07T13:36:15Z

gateleen-queue/src/main/resources/gateleen_queue_splitter_configuration_schema

You could also add unit tests for the schema validation. See KafkaTopicConfigurationSchemaValidationTest as example

mcweba · 2024-02-07T13:45:36Z

...een-queue/src/main/java/org/swisspush/gateleen/queue/queuing/splitter/QueueSplitterImpl.java

+        return promise.future();
+    }
+
+    public boolean isInitialized() {


What do you need

public boolean isInitialized() { return initialized; }

for? Only for tests? What happens if you remove the config resource and initialized=false is set?

Yes, the method is used only in QueueSplitterImpl tests to verify the current configuration is valid. Same purpose as KafkaHandler.isInitialized() also used only in KafkaHandler tests.

zorba71 added 14 commits January 22, 2024 11:40

#550 added QueueSplitterConfigurationParser

dffde37

#550 added QueueSplitter and used in QueuingHandler

761da46

#550 extracted interface QueueSplitter with also NoOp implementation

2c28f27

#550 added split executors

c385935

#550 implemented QueueSplitterImpl

12b48f1

merged develop in feature/550_queue_splitter

57c3987

#550 converted postfixFromUrl in QueueSplitterConfiguration in Pattern

a834584

#550 removed method getQueueSplitExecutors() from QueueSplitterImpl

1446d05

#550 commited some tests to review

9ab5c6f

#550 fixed log

46ffb4c

Merge branch 'develop' into feature/550_queue_splitter

bf32a9c

#550 added in playground examples of splitters

3739ea3

#550 reviewed tests

00b1b03

#550 refactoring

4a4eecd

mcweba self-requested a review February 6, 2024 10:33

#550 added test NoOpQueueSplitterTest

7f30b4b

mcweba requested changes Feb 6, 2024

View reviewed changes

#550 fixes, refactorings and improvements in splitter

ba3306b

mcweba reviewed Feb 7, 2024

View reviewed changes

zorba71 and others added 4 commits February 7, 2024 18:31

#550 fixes, refactorings and improvements in splitter

4f6d4c9

#550 added test to verify header matching is case insensitive

d393920

#550 extended readme

318c2ae

Update README_queue.md

c6597cd

mcweba approved these changes Feb 9, 2024

View reviewed changes

gcastaldi merged commit 161ce36 into develop Feb 9, 2024
6 checks passed

gcastaldi deleted the feature/550_queue_splitter branch February 9, 2024 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/550 queue splitter #557

Feature/550 queue splitter #557

gcastaldi commented Feb 6, 2024

codecov bot commented Feb 6, 2024 •

edited

Loading

mcweba Feb 6, 2024

mcweba Feb 6, 2024

mcweba Feb 6, 2024

mcweba Feb 6, 2024

mcweba Feb 6, 2024

mcweba Feb 6, 2024

gcastaldi Feb 7, 2024

mcweba Feb 6, 2024

mcweba Feb 7, 2024

mcweba Feb 7, 2024

mcweba Feb 7, 2024

mcweba Feb 7, 2024

gcastaldi Feb 7, 2024

		@@ -0,0 +1,32 @@
		{
		"$schema": "http://json-schema.org/draft-04/schema#",

Feature/550 queue splitter #557

Feature/550 queue splitter #557

Conversation

gcastaldi commented Feb 6, 2024

codecov bot commented Feb 6, 2024 • edited Loading

Codecov Report

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

codecov bot commented Feb 6, 2024 •

edited

Loading