HSEARCH-1261 - ArrayIndexOutOfBounds in JGroups backend AutoNodeSelector #376

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
@@ -0,0 +1,138 @@
+package org.hibernate.search.test.backends.jgroups;
@Sanne
Sanne Jan 20, 2013

Some training: all sources need a copyright header; For historical reasons (copyright headers can't be updates for legal reasons) not all headers are the same in the project as some use an older wording. So you should find a recent one and configure your IDE to use it as a template. Note that Hibernate and Infinispan use a different header.

(no need to fix this now, I'll do that)

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * JGroups' Auto Node Selector Test
+ * <p/>
+ * Test if the selector choose a valid member
+ *
+ * @author Pedro Ruivo
+ * @since 4.3
+ */
+public class AutoNodeSelectorTest {
+
+ private static final String NEGATIVE_HASH_CODE_INDEX_NAME = "test.Book";
+ private static final String POSITIVE_HASH_CODE_INDEX_NAME = "test.Author";
+ private static final String ZERO_HASH_CODE_INDEX_NAME = "";
+ private static final AtomicInteger NEXT_ADDRESS_ID = new AtomicInteger(0);
@Sanne
Sanne Jan 20, 2013

the Hibernate formatting style would use spaces around arguments:

private static final AtomicInteger NEXT_ADDRESS_ID = new AtomicInteger( 0 );
@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+ * Test if the selector choose a valid member
+ *
+ * @author Pedro Ruivo
+ * @since 4.3
+ */
+public class AutoNodeSelectorTest {
+
+ private static final String NEGATIVE_HASH_CODE_INDEX_NAME = "test.Book";
+ private static final String POSITIVE_HASH_CODE_INDEX_NAME = "test.Author";
+ private static final String ZERO_HASH_CODE_INDEX_NAME = "";
+ private static final AtomicInteger NEXT_ADDRESS_ID = new AtomicInteger(0);
+ private static final AtomicLong NEXT_VIEW_ID = new AtomicLong(0);
+
+ @Test
+ public void testIndexNameUsed() {
+ assert NEGATIVE_HASH_CODE_INDEX_NAME.hashCode() < 0;
@Sanne
Sanne Jan 20, 2013

assert won't work without enabling assertions on the JVM. Some frameworks/IDEs do that automatically, but it's a better practice to use the Assert utilities from JUnit.
Don't let you be fooled by Infinispan: they are wrong :)

@Sanne Sanne commented on the diff Jan 20, 2013
...ate/search/backend/impl/jgroups/AutoNodeSelector.java
@@ -1,6 +1,6 @@
-/*
+/*
@Sanne
Sanne Jan 20, 2013

this is technically correct, but we have a strong policy of no-reformatting of code areas which are not related to the fix.
That's important to minimize conflicts across patches, as there are several different branches which will need to be maintained for very long time, so cherry-picking fixes but not all changes: we need to make sure that this future work will not be too hard to do.

@Sanne
Sanne Jan 20, 2013

If you see obvious whitespace / formatting / comments typos a fix is very welcome, just use a separate commit for it so that we can choose which ones to apply to maintenance branches.

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+public class AutoNodeSelectorTest {
+
+ private static final String NEGATIVE_HASH_CODE_INDEX_NAME = "test.Book";
+ private static final String POSITIVE_HASH_CODE_INDEX_NAME = "test.Author";
+ private static final String ZERO_HASH_CODE_INDEX_NAME = "";
+ private static final AtomicInteger NEXT_ADDRESS_ID = new AtomicInteger(0);
+ private static final AtomicLong NEXT_VIEW_ID = new AtomicLong(0);
+
+ @Test
+ public void testIndexNameUsed() {
+ assert NEGATIVE_HASH_CODE_INDEX_NAME.hashCode() < 0;
+ assert ZERO_HASH_CODE_INDEX_NAME.hashCode() == 0;
+ assert POSITIVE_HASH_CODE_INDEX_NAME.hashCode() > 0;
+ }
+
+ @Test
@Sanne
Sanne Jan 20, 2013

Hibernate uses TABs for formatting instead of the Infinispan's "three spaces". Conventions ..

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+ addressId = in.readInt();
+ }
+
+ @Override
+ public void writeTo(DataOutput dataOutput) throws Exception {
+ dataOutput.writeInt(addressId);
+ }
+
+ @Override
+ public void readFrom(DataInput dataInput) throws Exception {
+ addressId = dataInput.readInt();
+ }
+
+ @Override
+ public String toString() {
+ return "TestAddress{" +
@Sanne
Sanne Jan 20, 2013

It's ok to break longer lines, but don't be too aggressive. Readability is an important value: you won't find a strict rule about "how many columns is the threshold": just look at it and use your sensibility for where a break is more appropriate and clear.

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+import java.io.IOException;
+import java.io.ObjectInput;
+import java.io.ObjectOutput;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * JGroups' Auto Node Selector Test
+ * <p/>
+ * Test if the selector choose a valid member
+ *
+ * @author Pedro Ruivo
+ * @since 4.3
+ */
@Sanne
Sanne Jan 20, 2013

I'm adding:

@TestForIssue(jiraKey="HSEARCH-1261")
@Sanne
Sanne Jan 20, 2013

(that's another Hibernate convention)

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+ for (int viewSize = 1; viewSize <= 50; ++viewSize) {
+ View view = createView(viewSize);
+ assert view.getMembers().size() == viewSize;
+ selector.viewAccepted(view);
+ }
+ }
+
+ private View createView(int size) {
+ List<Address> addressList = new LinkedList<Address>();
+ while (size-- > 0) {
+ addressList.add(new TestAddress(NEXT_ADDRESS_ID.incrementAndGet()));
+ }
+ return new View(addressList.get(0), NEXT_VIEW_ID.incrementAndGet(), addressList);
+ }
+
+ private final class TestAddress implements Address {
@Sanne
Sanne Jan 20, 2013

this class can be static

@Sanne Sanne commented on the diff Jan 20, 2013
...earch/test/backends/jgroups/AutoNodeSelectorTest.java
+ while (size-- > 0) {
+ addressList.add(new TestAddress(NEXT_ADDRESS_ID.incrementAndGet()));
+ }
+ return new View(addressList.get(0), NEXT_VIEW_ID.incrementAndGet(), addressList);
+ }
+
+ private final class TestAddress implements Address {
+
+ private int addressId;
+
+ public TestAddress(int addressId) {
+ this.addressId = addressId;
+ }
+
+ @SuppressWarnings("UnusedDeclaration")
+ public TestAddress() {
@Sanne
Sanne Jan 20, 2013

(the warning looks correct, should not be suppressed)

@Sanne
Hibernate member

Nice test!
The commit message shoulr start with the issue code, so without the star. Also there is no need to separate the issue code from the main description.

@Sanne
Hibernate member

merging..

@Sanne Sanne closed this Jan 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment