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

Adding an ability to use a default implementation for sending history. #5347

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

dmitrii-beliakov
Copy link
Collaborator

@dmitrii-beliakov dmitrii-beliakov commented Aug 24, 2023

I extracted interfaces from AbstractBolt, this allows to have an interface with the default implementation for sending history. I chose it to be an interface and not a class in the hierarchy of bolts, so that we can opt-in or opt-out the default implementation more easily. Also it was needed to move classes to another module, up in the dependency structure.
I added the usage to one bolt to show how it is going to look like.

Although is solves code duplication problem, there are some downsides:

  • this interface cannot enforce a topology to declare what is needed to be declared.
  • the access modifiers for some methods changed to be public (I actually don't know why and don't really see the need for it, the class that extends one of the abstract bolts is not inherited and used only in topologies. This is not super clear point. Bolts could be final to resolve this).
  • there is a need to unify the naming of streams and bolts for history.

Copy link
Collaborator

@IvanChupin IvanChupin left a comment

Choose a reason for hiding this comment

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

Great job! The approach to move save history methods into one separate interface looks really convenient.

@@ -66,10 +66,10 @@ public abstract class AbstractBolt extends BaseRichBolt {
@Getter(AccessLevel.PROTECTED)
private transient Integer taskId;

@Getter(AccessLevel.PROTECTED)
@Getter(AccessLevel.PUBLIC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, you decided to point out explicitly AccessLevel.PUBLIC only to keep the common behaviour here, like as it is in the cases for AccessLevel.PROTECTED?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a part of an interface now, so it has to be public

Comment on lines +29 to +31
default String getHistoryStreamName() {
return "HUB_TO_HISTORY_TOPOLOGY_SENDER";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be more convenient to keep HUB_TO_HISTORY_TOPOLOGY_SENDER constant in the Stream enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I have a thought that it needs to be in some place that is accessible in the bolts and the service at the same time. We need this constant when configuring topologies, so it would be convenient to have some unified approach of adding. If it is a helper method then it can't be enum that we have in the topology package.
This is a task to think about. But once it is done, we can change this method without changing the rest of the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think getHistoryStreamName() must be used in declareOutputFields

@dmitrii-beliakov dmitrii-beliakov removed their assignment May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants