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

StartServer should be renamed. #12791

Closed
dbrimley opened this issue Apr 5, 2018 · 15 comments · Fixed by #13818
Closed

StartServer should be renamed. #12791

dbrimley opened this issue Apr 5, 2018 · 15 comments · Fixed by #13818

Comments

@dbrimley
Copy link
Member

@dbrimley dbrimley commented Apr 5, 2018

When using the start scripts in the bin directory, from zip distribution.

The class to start a Hazelcast Member is called...

com.hazelcast.core.server.StartServer

When you run a JPS command it will just show StartServer.

In Technical documentation we call these Members.

We should rename the class to be...

com.hazelcast.core.member.StartHazelcastMember

In this way it complies with our naming standards and it also makes things more intuitive when reading process lists.

@ahmetmircik

This comment has been minimized.

Copy link
Member

@ahmetmircik ahmetmircik commented Apr 5, 2018

liked this issue, maybe we can even rename it to HazelcastMemberStarter like JUnits' JUnitStarter

@tkountis

This comment has been minimized.

Copy link
Contributor

@tkountis tkountis commented Apr 5, 2018

Why not plain old HazelcastMember? When I see a process list, I expect to see the name of the process. The Start or Starter prefix/suffix, even though make sense on the code side, they aren't intuitive to the admin of the system. @dbrimley @ahmetmircik WDYT?

@emre-aydin

This comment has been minimized.

Copy link
Contributor

@emre-aydin emre-aydin commented Apr 5, 2018

+1 for HazelcastMember

@ahmetmircik

This comment has been minimized.

Copy link
Member

@ahmetmircik ahmetmircik commented Apr 5, 2018

@tkountis i think it depends on how you look it. I see it as an abstraction to start a member with, in the future some other capability(don't know what they are now) may also be added. But i'm ok with any naming if i can see Hazelcast in the name.

@pveentjer

This comment has been minimized.

Copy link
Member

@pveentjer pveentjer commented Apr 6, 2018

This isn't critical for the 3.10 release. I'm proposing to get it moved to 3.11.

@eminn

This comment has been minimized.

Copy link
Collaborator

@eminn eminn commented Apr 6, 2018

Note to the future implementer: Please double check all the starter scripts in the docker images since most of them uses that class.

@mmedenjak

This comment has been minimized.

Copy link
Contributor

@mmedenjak mmedenjak commented Apr 9, 2018

@dbrimley can this be moved to 3.11?

@pveentjer

This comment has been minimized.

Copy link
Member

@pveentjer pveentjer commented Apr 10, 2018

Since this isn't critical for the 3.10 release, moving it to the 3.11 release.

@mesutcelik

This comment has been minimized.

Copy link
Contributor

@mesutcelik mesutcelik commented Sep 12, 2018

This needs to be renamed in all hazelcast docker images and start scripts as @eminn mentioned.
What is the latest decision of the name here @dbrimley ?

@dbrimley

This comment has been minimized.

Copy link
Member Author

@dbrimley dbrimley commented Sep 12, 2018

I worry that HazelcastMember may convey something unintended to the user. As @ahmetmircik says, its intent should be that it is a starter for a hazelcast member. It's not THE HazelcastMember, it may cause confusion then with HazelcastInstance

I'd like to call it HazelcastMemberStarter, I admit its not as clean and nice in the process list but it at least is better than we have.

@leszko

This comment has been minimized.

Copy link
Contributor

@leszko leszko commented Sep 21, 2018

I've created the related PRs:

Keep in mind the PRs were the easy part, there are a lot to things related to be done after the release:

  • Dockerfiles in hazelcast-docker
  • Dockerfile in hazelcast-openshift
  • hazelcast-code-samples
  • hazelcast-qe
  • something more?

What's more, users may have StartServer somewhere in their scripts, so we may break it.

Saying that I would re-consider if we really want to rename it. After all HazelcastMemeberStarter is not so much better than StartServer IMO.

@mmedenjak @jerrinot , What are your thoughts?

@dbrimley

This comment has been minimized.

Copy link
Member Author

@dbrimley dbrimley commented Sep 21, 2018

@leszko I guess the problem we're trying to solve here is making the process identifiable as a Hazelcast Member. Who knows what StartServer is.

@dbrimley

This comment has been minimized.

Copy link
Member Author

@dbrimley dbrimley commented Sep 21, 2018

For backwards compatibility we could keep the original class and duplicate it with the new name and change our start scripts to use it. Other 3rd party scripts would then still work?

@mmedenjak

This comment has been minimized.

Copy link
Contributor

@mmedenjak mmedenjak commented Sep 21, 2018

I agree with @dbrimley . I am against breaking compatibility. We try so hard to keep it everywhere else to break it so easily here.

@leszko

This comment has been minimized.

Copy link
Contributor

@leszko leszko commented Sep 21, 2018

SGTM 👍

Added StartServer to the PRs for the backwards-compatibility. @mmedenjak PTAL.

@dbrimley dbrimley modified the milestones: 3.11, 3.12 Sep 24, 2018
@leszko leszko removed this from the 3.12 milestone Mar 1, 2019
@mmedenjak mmedenjak added this to the 4.0 milestone Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.