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 hazelcast property #281

Merged

Conversation

SaitTalhaNisanci
Copy link
Contributor

@SaitTalhaNisanci SaitTalhaNisanci commented Jun 18, 2018

This PR adds Hazelcast Property as explained in #280.

It also removes some of the methods from config that do not exist in Java and uses Hazelcast Property instead.

fixes #280.

)

// Hazelcast is a struct for client properties.
type Hazelcast struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the name HazelcastProperty is much more suitable for this struct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In go since dotimport is not used, it will be imported as property.Hazelcast vs property.HazelcastProperty do you think it looks bad ?

This is from go package names:

Avoid stutter. Since client code uses the package name as a prefix when referring to the package contents, the names for those contents need not repeat the package name. The HTTP server provided by the http package is called Server, not HTTPServer. Client code refers to this type as http.Server, so there is no ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I read this source code with type Hazelcast, I automatically expect to see methods like Hazelcast.newHazelcastInstance/shutdownAll etc..
This is what we are used to see from Java API.
I get that golang convention is different also. I personally prefer the HazelcastProperty instead.
Lets get more opinion on this ? @asimarslan wdyt ?

}

// NewHazelcastPropertiesWithConfig returns HazelcastProperties with the given config's properties.
func NewHazelcastPropertiesWithConfig(config *config.Config) *HazelcastProperties {
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor does not seem really necessary. I think caller should call NewHazelcastPropertiesWithProperties , and pass the properties instead of config always.

@SaitTalhaNisanci SaitTalhaNisanci force-pushed the feature/hazelcastProperty branch 3 times, most recently from 5cd6a38 to 196ef20 Compare June 20, 2018 09:37
@SaitTalhaNisanci SaitTalhaNisanci merged commit 94d313b into hazelcast:master Jun 22, 2018
0.4 automation moved this from In progress to Done Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
0.4
  
Done
Development

Successfully merging this pull request may close these issues.

Hazelcast Property
3 participants