Databus Throttling Changes #847

nabajyotiDash-hub · 2024-11-04T15:00:23Z

Github Issue

What Are We Doing Here?

Here is where you should describe the problem you are solving as it relates to the Github Issue referred above, adding any fine
details on the solution that might otherwise not be recognizable for someone
unfamiliar with the changes. Add some pictures if it helps.

How to Test and Verify

Check out this PR
Run Command X, Click Button Y
Profit

Risk

Level

Low, Medium, or High. Give an indication of what you think is the level of change introduced by this PR. High means a massive change to a core functionality.
Low means a really minor change that shouldn't have any regression effect.

Required Testing

Smoke, Regression, or Manual. (All changes except documentation need smoke
testing at a minimum).

Risk Summary

Add one or a few complete sentences about the possible risks or concerns for
this change.

Code Review Checklist

Tests are included. If not, make sure you leave us a line or two for the reason.
Pulled down the PR and performed verification of at least being able to
build and run.
Well documented, including updates to any necessary markdown files. When
we inevitably come back to this code it will only take hours to figure out, not
days.
Consistent/Clear/Thoughtful? We are better with this code. We also aren't
a victim of rampaging consistency, and should be using this course of action.
We don't have coding standards out yet for this project, so please make sure to address any feedback regarding STYLE so the codebase remains consistent.
PR has a valid summary, and a good description.

1. /UpdateParameterStore 2. /QueueExecutionAttributes/<queue_type>/<queue_name>

sor/pom.xml

sor/src/main/java/com/bazaarvoice/emodb/sor/core/DefaultDataStore.java

* PD-257044-changes-for-databus * duplicated kafka and ssm util packages in emo sor module * removed redundant dependencies

* PD-257044-changes-for-databus * duplicated kafka and ssm util packages in emo sor module * removed redundant dependencies * additional changes * additional changes * additional changes * removed unneccessary dependencies

This reverts commit 62dc31e.

quality/integration/src/test/java/test/integration/auth/TableRoleManagerDAOTest.java

queue-api/src/main/java/com/bazaarvoice/emodb/queue/api/DedupQueueService.java

queue-api/src/main/java/com/bazaarvoice/emodb/queue/api/QueueService.java

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

queue/src/test/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionServiceTest.java

sor-api/src/main/java/com/bazaarvoice/emodb/sor/api/Names.java

web/src/main/java/com/bazaarvoice/emodb/web/resources/queue/QueueResource1.java

web/src/main/java/com/bazaarvoice/emodb/web/resources/sor/DataStoreResource1.java

sor/src/main/java/com/bazaarvoice/emodb/sor/core/DefaultDataStore.java

bharadwaj-k

LGTM

IgorKruiper

one potential blocking issue

databus/src/main/java/com/bazaarvoice/emodb/databus/core/OwnerAwareDatabus.java

queue/src/main/java/com/bazaarvoice/emodb/queue/core/AbstractQueueService.java

sor/src/main/java/com/bazaarvoice/emodb/sor/core/DefaultDataStore.java

IgorKruiper

Thanks

vedabalanr · 2024-11-06T17:58:04Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/kafka/KafkaProducerService.java

@@ -46,7 +46,7 @@ public void sendMessages(String topic, Collection<String> events, String queueTy
                throw new RuntimeException("Error sending messages to Kafka", e);
            }
        }
-        _log.info("Finished sending messages to topic '{}' time taken : {} milliseconds", topic, Duration.between(startTime,LocalDateTime.now()).toMillis());
+        _log.info("Finished sending messages to topic '{}' time taken : {} milliseconds", topic, Duration.between(startTime, LocalDateTime.now()).toMillis());


do log the events.size(O as well in this log, will be easy for analytics, on time taken per topic for the events size...

vedabalanr · 2024-11-06T17:59:31Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/ssm/ParameterStoreUtil.java

+            return response.getVersion();
+        } catch (Exception e) {
+            logger.error("Failed to update parameter: " + key + " with value: " + value, e);
+            throw new RuntimeException("Unexpected error updating parameter: " + key + " with value: " + value, e);


Why its a RuntimeException and not an Exception?

vedabalanr · 2024-11-06T18:00:03Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/ssm/ParameterStoreUtil.java

+            logger.info("Successfully updated parameter: " + key + " with value: " + value + ", Update Version: " + response.getVersion());
+            return response.getVersion();
+        } catch (Exception e) {
+            logger.error("Failed to update parameter: " + key + " with value: " + value, e);


Is this log required? adding too much log events... anyway the thrown exception will be logged by the caller/catcher...

vedabalanr · 2024-11-06T18:05:57Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

+        }
+        executionName = executionName.trim();
+        // Replace invalid characters with underscores
+        String sanitized = executionName.replaceAll("[^a-zA-Z0-9\\-_]", "_");


suggestion: to optimize
replaceAll("[^a-zA-Z0-9\\-_]+", "")
a '+' after the square bracket would be better, as it will reduce the consecutive invalid character to one '' string...
"ab_2@@3###4$$$$5%%%%%".replaceAll("[^a-zA-Z0-9\\-_]", "") would return 'ab_2___3____4_____5___'

"ab_2@@_3###4$$$$5%%%%%".replaceAll("[^a-zA-Z0-9\\-_]+", "") would return 'ab_2__3__4__5'

vedabalanr · 2024-11-06T18:09:52Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

+
+        // Truncate from the beginning if length exceeds 66 characters
+        if (sanitized.length() > 66) {
+            sanitized = sanitized.substring(sanitized.length() - 66);


Is the sanitized text expected to be unique?
If so why not append a GUID value to the string, after trimming to 50 characters?

vedabalanr · 2024-11-06T18:12:27Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

+                return;
+            } catch(Exception e){
+                logger.error("Error starting step-function with attributes " + queueExecutionAttributes + " | " + e.getMessage());
+                throw new RuntimeException("Error starting step-function with attributes " + queueExecutionAttributes + " | " + e.getMessage());


Why not throw the exception and why thrownig a RuntimeException?

vedabalanr · 2024-11-06T18:15:27Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/Entities/QueueExecutionAttributes.java

+    private Integer queueThreshold;
+    private Integer batchSize;
+    private Integer interval;
+    private String status;


Status should be a boolean or ENUM, random string will be confusing and maintenance hassle

vedabalanr · 2024-11-06T18:17:12Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/Entities/QueueExecutionAttributes.java

+    private String queueName;
+    private String queueType;
+    private String topicName;
+    private Integer queueThreshold;


Why is this an Object (Integer) and not a primitive 'integer'?
I suppose this should be a 'long', to support really long size value

vedabalanr · 2024-11-06T18:18:53Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

+        }
+
+        if(executionAttributes.getStatus() == null || executionAttributes.getStatus().isEmpty()) {
+            executionAttributes.setStatus("ENABLED");


Use Enum for status, to have the predefined list of values and easy comparison/check

vedabalanr · 2024-11-06T18:20:01Z

queue/src/main/java/com/bazaarvoice/emodb/queue/core/stepfn/StepFunctionService.java

+        } catch (StateMachineDoesNotExistException e) {
+            logger.error("State Machine does not exist for queue_type: " + queueType + ", queue_name: " + queueName, e);
+        } catch (InvalidArnException e) {
+            logger.error("Invalid ARN provided for queue_type: " + queueType + ", queue_name: " + queueName, e);


Why not log the ARN?

rishuyadavbv and others added 5 commits November 4, 2024 17:53

feat: execution name sanitization and unit test for stepfn

cce3d28

master_size api changes

49cfe91

Admin API changes (#845)

f082d99

1. /UpdateParameterStore 2. /QueueExecutionAttributes/<queue_type>/<queue_name>

PD-257044-changes-for-databus (#846)

05fb5ed

added datadog metrics changes

62dc31e

nabajyotiDash-hub requested a review from bharadwaj-k November 4, 2024 15:00

nabajyotiDash-hub requested a review from a team as a code owner November 4, 2024 15:00

nabajyotiDash-hub changed the title ~~Master2~~ Databus Throttling Changes Nov 4, 2024

bharadwaj-k reviewed Nov 5, 2024

View reviewed changes

sor/pom.xml Outdated Show resolved Hide resolved

sor/src/main/java/com/bazaarvoice/emodb/sor/core/DefaultDataStore.java Show resolved Hide resolved

anurag0510 and others added 6 commits November 5, 2024 16:03

[PD-257044] databus changes rebase (#848)

72ed737

* PD-257044-changes-for-databus * duplicated kafka and ssm util packages in emo sor module * removed redundant dependencies

admin api and queue name split

0dad904

Pd 257044 databus changes rebase (#849)

63d05f0

* PD-257044-changes-for-databus * duplicated kafka and ssm util packages in emo sor module * removed redundant dependencies * additional changes * additional changes * additional changes * removed unneccessary dependencies

Revert "added datadog metrics changes"

be3df07

This reverts commit 62dc31e.

build fix

6b0b672

endpoint change and databus changes

6fab83e

bharadwaj-k reviewed Nov 6, 2024

View reviewed changes

anurag0510 and others added 2 commits November 6, 2024 05:41

updated updateRefInDatabus method

ff905b0

address review commnets

02c3a35

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

branch admin -prepare release emodb-6.5.205

7d71b48

bv-jenkins dismissed bharadwaj-k’s stale review via 7d71b48 November 6, 2024 06:43

branch admin -prepare for next development iteration

6193999

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

nabajyotiDash-hub enabled auto-merge (squash) November 6, 2024 07:12

nabajyotiDash-hub disabled auto-merge November 6, 2024 07:16

nabajyotiDash-hub enabled auto-merge (squash) November 6, 2024 07:16

nabajyotiDash-hub requested a review from anandujayan November 6, 2024 07:24

anandujayan previously approved these changes Nov 6, 2024

View reviewed changes

edit doc

05cfcbf

nabajyotiDash-hub dismissed anandujayan’s stale review via 05cfcbf November 6, 2024 07:34

nabajyotiDash-hub dismissed bharadwaj-k’s stale review via 05cfcbf November 6, 2024 07:34

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

fix tests

1861f8b

nabajyotiDash-hub dismissed bharadwaj-k’s stale review via 1861f8b November 6, 2024 07:58

anandujayan previously approved these changes Nov 6, 2024

View reviewed changes

fix tests

da60cdb

nabajyotiDash-hub dismissed anandujayan’s stale review via da60cdb November 6, 2024 08:18

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

IgorKruiper suggested changes Nov 6, 2024

View reviewed changes

fix review comments

a40a48c

nabajyotiDash-hub dismissed bharadwaj-k’s stale review via a40a48c November 6, 2024 11:34

removed ;

ab2c2d1

IgorKruiper approved these changes Nov 6, 2024

View reviewed changes

jenkins and others added 5 commits November 6, 2024 13:28

branch admin -prepare release emodb-6.5.206

6e1cf00

upgrade pom version

3373932

branch admin -prepare release emodb-6.5.208

2941007

branch admin -prepare for next development iteration

54122d9

get or default change

173f50b

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

sor get or default changes

13847e8

anandujayan dismissed bharadwaj-k’s stale review via 13847e8 November 6, 2024 16:03

bharadwaj-k previously approved these changes Nov 6, 2024

View reviewed changes

Unit test fixes

fcd125a

anandujayan dismissed bharadwaj-k’s stale review via fcd125a November 6, 2024 17:27

bharadwaj-k approved these changes Nov 6, 2024

View reviewed changes

nabajyotiDash-hub merged commit 387db36 into main Nov 6, 2024
3 of 5 checks passed

nabajyotiDash-hub deleted the master2 branch November 6, 2024 17:40

vedabalanr reviewed Nov 6, 2024

View reviewed changes

Databus Throttling Changes #847

Databus Throttling Changes #847

Uh oh!

Conversation

nabajyotiDash-hub commented Nov 4, 2024

Github Issue

What Are We Doing Here?

How to Test and Verify

Risk

Level

Required Testing

Risk Summary

Code Review Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bharadwaj-k left a comment

Choose a reason for hiding this comment

Uh oh!

IgorKruiper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IgorKruiper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!