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

Add ReconnectSenderWrapper #6338

Merged
merged 6 commits into from
Feb 23, 2024
Merged

Add ReconnectSenderWrapper #6338

merged 6 commits into from
Feb 23, 2024

Conversation

nielsm5
Copy link
Sponsor Member

@nielsm5 nielsm5 commented Feb 22, 2024

No description provided.

Co-authored-by: J. Koster <j.koster@gmx.com>
*
* @author Niels Meijer
*/
public class RuntimeSenderWrapper extends SenderWrapperBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the name because it doesn't tell you what it does, but I don't have a better name yet.

  • OpenOnDemandSenderWrapper?
  • ConnectOnDemandSenderWrapper?
  • RuntimeOpenSenderWrapper?
  • DynamicOpenSenderWrapper?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree with you. I like this one the best ConnectOnDemandSenderWrapper or what about OpenCloseSenderWrapper? That's exactly what happens. Perhaps let Sena validate it, since she is exactly a person who directly needs to understand it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nielsm5 What do you think of these naming suggestions?
ConnectOnDemandSenderWrapper is the best name I can come up with I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

ReconnectingSenderWrapper

…pper.java

Co-authored-by: Tim van der Leeuw <tnleeuw@gmail.com>
@nielsm5 nielsm5 requested a review from tnleeuw February 23, 2024 13:23
Copy link
Contributor

@tnleeuw tnleeuw left a comment

Choose a reason for hiding this comment

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

Well I guess the name is not going to be changed to then Appove 😁

@nielsm5
Copy link
Sponsor Member Author

nielsm5 commented Feb 23, 2024

Well I guess the name is not going to be changed to then Appove 😁

I'll quickly interview some people.. So far I'm not convinced yet. (At least the documentation/implementation is ok now)

@tnleeuw
Copy link
Contributor

tnleeuw commented Feb 23, 2024

Well I guess the name is not going to be changed to then Appove 😁

I'll quickly interview some people.. So far I'm not convinced yet. (At least the documentation/implementation is ok now)

I'm also not sure either of these suggestions if really right but to me they're better than RuntimeSenderWrapper. Either way, changing or not changing will not change my approval I just wanted to hear some reaction from you on that comment.

Copy link

sonarcloud bot commented Feb 23, 2024

@nielsm5 nielsm5 merged commit cafd86f into master Feb 23, 2024
17 checks passed
@nielsm5 nielsm5 deleted the feature/runtimeSender branch February 23, 2024 15:05
@nielsm5 nielsm5 changed the title Add RuntimeSenderWrapper Add ReconnectSenderWrapper Feb 23, 2024
nielsm5 added a commit that referenced this pull request Feb 23, 2024
(cherry picked from commit cafd86f)
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.

Add a SenderWrapper that handles connections runtime and not during the default lifecycle states
4 participants