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

ISPN-8912 SoftIndexFileStore does not fail gracefully if max node sizee exceeds 32767 #5819

Merged
merged 1 commit into from Mar 13, 2018

Conversation

@rvansa
Copy link
Member

rvansa commented Mar 6, 2018

  • validate the configuration
  • use shorts where the value should be limited to short
  • use i18d logging
  • minor code cleanup based on IDE suggestions

https://issues.jboss.org/browse/ISPN-8912

…e exceeds 32777

* validate the configuration
* use shorts where the value should be limited to short
* use i18d logging
* minor code cleanup based on IDE suggestions
@rvansa rvansa requested a review from ryanemerson Mar 6, 2018
Copy link
Contributor

ryanemerson left a comment

Overall LGTM, just a couple of nitpicks about short usage.

List<IndexSpace> list = entry.getValue();
int requiredSize = 8 + list.size() * 10;
buffer = buffer.capacity() < requiredSize ? ByteBuffer.allocate(requiredSize) : buffer;
buffer.position(0);
buffer.limit(requiredSize);
// TODO: change this to short

This comment has been minimized.

Copy link
@ryanemerson

ryanemerson Mar 9, 2018

Contributor

Did you forget about this?

This comment has been minimized.

Copy link
@rvansa

rvansa Mar 12, 2018

Author Member

No; I didn't want to change binary format in some micro - the plan is to add some version numbers both to index and to data files, and then I'll do the change to binary format as well.

@@ -389,7 +376,9 @@ private void loadFreeBlocks(long freeBlocksOffset) throws IOException {
if (!read(indexFile, buffer)) {
throw new IOException("Cannot read free blocks lists!");
}
// TODO: change this to short

This comment has been minimized.

Copy link
@ryanemerson
private short headerLength() {
int headerLength = INNER_NODE_HEADER_SIZE + prefix.length;
assert headerLength <= Short.MAX_VALUE;
return (short) headerLength;
}

private int contentLength() {

This comment has been minimized.

Copy link
@ryanemerson

ryanemerson Mar 9, 2018

Contributor

Should be short

@wburns
wburns approved these changes Mar 9, 2018
Copy link
Member

wburns left a comment

Just minor nitpick re: interruption which doesn't matter much.

@@ -241,7 +240,8 @@ public void stop() {
indexQueue = null;
storeQueue = null;
} catch (InterruptedException e) {
throw new PersistenceException("Cannot stop cache store", e);
Thread.currentThread().interrupt();

This comment has been minimized.

Copy link
@wburns

wburns Mar 9, 2018

Member

I wouldn't normally expect to reinterpret the thread when throwing an exception that stating that we were interrupted. I find I would normally reset the status and return early or throw the exception, but not both.

This comment has been minimized.

Copy link
@rvansa

rvansa Mar 12, 2018

Author Member

OK, I have thought the best practice is to set thread status to interrupted if the thrown exception is not directly InterruptedException.

This comment has been minimized.

Copy link
@wburns

wburns Mar 13, 2018

Member

Yeah I was thinking it was a different exception. Seems good.

@wburns wburns merged commit 019e290 into infinispan:master Mar 13, 2018
1 check failed
1 check failed
continuous-integration/jenkins/pr-head This commit has test failures
Details
@wburns

This comment has been minimized.

Copy link
Member

wburns commented Mar 13, 2018

Integrated into master, thanks @rvansa !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.