Fix for IBM JDK 6 #5312
Fix for IBM JDK 6 #5312
Conversation
|
We should not add possible fixes. Either it is a fix or it isn't. |
|
I can't reproduce, I can't fix it surely. |
|
I understand it is difficult to fix a problem that is difficult to reproduce. But who says that this fixes the problem? Perhaps it causes new problems. |
|
Well at least it fixes the Iterator contract which doesn't require to call Iterator::hasNext before Iterator::next and I guess on IBM that might sometimes happen (depending on the JIT but I can't make it happen locally). |
|
I don't see any tests added. And afaik the nature of next has changed since it now throws a NoSuchElementException. So we need to have additional tests for these IteratorFactories. |
|
http://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html#next() this is what the spec defines |
|
I know how the next works :) But we need to make sure we follow good practices when making code changes, and adding tests is one of them. |
| @@ -49,6 +50,18 @@ public boolean hasNext() { | |||
|
|
|||
| @Override | |||
| public Map.Entry<K, V> next() { | |||
| if (entry == null) { | |||
| while (iterator.hasNext()) { | |||
There was a problem hiding this comment.
- Shouldn't we set null to entry before next() method returns a value?
|
I think this PR should have corresponding unit tests for the iterators. |
| @@ -48,6 +49,18 @@ public boolean hasNext() { | |||
|
|
|||
| @Override | |||
| public K next() { | |||
| if (entry == null) { | |||
| while (iterator.hasNext()) { | |||
There was a problem hiding this comment.
I couldn't understand how this while loop works. Shouldn't it stop when a valid entry is found?
| @@ -240,7 +240,7 @@ | |||
| -Dhazelcast.test.use.network=false | |||
| </argLine> | |||
| <includes> | |||
| <include>**/**.java</include> | |||
| <include>com/hazelcast/replicatedmap/**.java</include> | |||
There was a problem hiding this comment.
This change should be reverted when it is about to merge.
There was a problem hiding this comment.
Yes it will be reverted, currently I'm just running the repmap tests in my own jenkins build and running all takes too much time :)
There was a problem hiding this comment.
I see the intention, I added note for in case of it is forgotten.
|
@Danny-Hazelcast can you verify the modified classes have 87.5% test coverage? |
|
verify |
|
seems this PR didn't fix the failures. ( at least for client side) |
|
oops I guess I haven't done anything about client side |
|
As I already mentioned above the bug is actually in the JRE implementation and fixed in newer version, therefore this is not a real bugfix but a workaround (in the test only!) for an old JRE bug. |
Fixed IBM Java 6 unittest failures. The problems are caused by the ArrayList implementation in that very version of J9. We might want to consider updating it, it is fixed in the latest version, probably the reason I couldn't reproduce it locally.