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

fixed multicast plugin bug and added test #8614

Merged
merged 1 commit into from
Jul 28, 2016

Conversation

bilalyasar
Copy link
Contributor

@bilalyasar bilalyasar commented Jul 28, 2016

I have rebased and opened new pr from this pr:
#8421

  • added javadoc
  • fixed small bug ( discovery property bug, invalid port check)
  • added test

@devOpsHazelcast
Copy link
Collaborator

Test PASSed.

@@ -47,7 +48,9 @@ public void setUp() {

System.setProperty(TestEnvironment.HAZELCAST_TEST_USE_NETWORK, "true");
System.setProperty("java.net.preferIPv4Stack", "true");
factory = createHazelcastInstanceFactory(2);
if (factory == null) {

Choose a reason for hiding this comment

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

Do we really need this? factory is already defined in HazelcastTestSupport so all cleanup, null checking etc. looks unnecessary to me.

I believe we should remove factory variable in this class and depend on the one @ HazelcastTestSupport. Good cleanup opportunity for you 👍

@mesutcelik
Copy link

👍 with minor comment.

Can you please add bug description as well? invalid port fix etc.

@bilalyasar
Copy link
Contributor Author

@mesutcelik reviewed your comments, also added description.

@ahmetmircik ahmetmircik added this to the 3.7 milestone Jul 28, 2016
@devOpsHazelcast
Copy link
Collaborator

Test PASSed.

@Donnerbart
Copy link
Contributor

👍

@Donnerbart Donnerbart merged commit bd4cfd1 into hazelcast:master Jul 28, 2016
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Source: Internal PR or issue was opened by an employee
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants