Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Databus Throttling Changes #847

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Databus Throttling Changes #847

wants to merge 22 commits into from

Conversation

nabajyotiDash-hub
Copy link
Contributor

Github Issue

1234

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

  1. Check out this PR
  2. Run Command X, Click Button Y
  3. 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.

@nabajyotiDash-hub nabajyotiDash-hub requested a review from a team as a code owner November 4, 2024 15:00
@nabajyotiDash-hub nabajyotiDash-hub changed the title Master2 Databus Throttling Changes Nov 4, 2024
anurag0510 and others added 6 commits November 5, 2024 16:03
* 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
bharadwaj-k
bharadwaj-k previously approved these changes Nov 6, 2024
bharadwaj-k
bharadwaj-k previously approved these changes Nov 6, 2024
Copy link

@bharadwaj-k bharadwaj-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

anandujayan
anandujayan previously approved these changes Nov 6, 2024
bharadwaj-k
bharadwaj-k previously approved these changes Nov 6, 2024
anandujayan
anandujayan previously approved these changes Nov 6, 2024
bharadwaj-k
bharadwaj-k previously approved these changes Nov 6, 2024
Copy link

@IgorKruiper IgorKruiper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one potential blocking issue

@@ -41,6 +41,9 @@ Subscription getSubscription(String ownerId, String subscription)
long getEventCount(String ownerId, String subscription)
throws UnauthorizedSubscriptionException;

public long getMasterEventCountUncached(String ownerId)
throws UnauthorizedSubscriptionException;;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throws UnauthorizedSubscriptionException;;
throws UnauthorizedSubscriptionException;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

// Check if queueType is "dedupq" and prepend "D" to execution name if true
String executionName = (queueType.equalsIgnoreCase("dedupq") ? "D_" : "") + queueName + "_" + timestamp;

String executionName = (queueType.equalsIgnoreCase("dedupq") ? "D_" : "") + queueName + "_" + timestamp;;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String executionName = (queueType.equalsIgnoreCase("dedupq") ? "D_" : "") + queueName + "_" + timestamp;;
String executionName = (queueType.equalsIgnoreCase("dedupq") ? "D_" : "") + queueName + "_" + timestamp;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. :)

if(getDataThrottlerValue())
_kafkaProducerService.sendMessages(MASTER_FANOUT_TOPIC, updateRefs, "update");
else
_eventWriterRegistry.getDatabusWriter().writeEvents(updateRefs);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue(blocker): this writeEvents function is called twice (line 760 and line 764) - I don't think that's desirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line got duplicated as part of rebasing. Thanks for noticing this.

Copy link

@IgorKruiper IgorKruiper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants