ISPN-12755 SoftIndexFileStore New SPI #9081
Conversation
|
Looks like my changes broke the migrator. Probably because it no longer supports segmentation. I need to see if I can fix this still and what to do. |
wburns
force-pushed
the
ISPN-12755_softindex_new_spi
branch
7 times, most recently
from
3a4dfc4
to
f3d4cc5
Compare
| * Returns an executor that will run the given tasks on a blocking thread as required. | ||
| * <p> | ||
| * Note that if the current thread is blocking, the task is invoked in the current thread, meaning the stage is | ||
| * always completed when returned, so any chained stage is also invoked on the current thread. |
There was a problem hiding this comment.
I think this might have been copy and pasted in error. A Executor only allows Runnable so no stages or chained stages.
There was a problem hiding this comment.
Sure, I can clean this up.
There was a problem hiding this comment.
Actually I forgot I changed this in my most recent work. I will update it to be the same.
| @@ -131,6 +132,7 @@ public void run() { | |||
| try { | |||
| scheduledFile = scheduledCompaction.poll(1, TimeUnit.MINUTES); | |||
| } catch (InterruptedException e) { | |||
| throw new CacheException(e); | |||
There was a problem hiding this comment.
Thread.currentThread().interrupt(); first?
There was a problem hiding this comment.
I can add it. Just an FYI though this code is completely removed in the next PR :)
| delay = AsyncProcessor.create(); | ||
| completeRequest(request); | ||
| return; | ||
| } else if (request.isResume()) { |
There was a problem hiding this comment.
Nitpick. No need for else if with return.
There was a problem hiding this comment.
It is still required. The task is a write based operation otherwise.
| currentOffset = 0; | ||
| logFile = null; | ||
| // TODO: Technically other writes won't be notified if sync write is enabled until | ||
| // the timeout above |
There was a problem hiding this comment.
Is there anything we can do this? Or do we need to remove the TODO and just comment the behaviour?
There was a problem hiding this comment.
I can't think of a better way to fix it. I can just comment that is the behavior. It is very similar to the behavior before.
| } catch (Exception e) { | ||
| queue.notifyError(); | ||
| throw new RuntimeException(e); | ||
| request.completeExceptionally(e); |
There was a problem hiding this comment.
Is it worth adding a debug log here?
There was a problem hiding this comment.
We could, it shouldn't really ever happen. I can add it.
| } | ||
|
|
||
| @Override | ||
| // TODO: this is probably broken, look into |
There was a problem hiding this comment.
The destroy method is never even invoked, even before my changes. I think i will just remove the method.
| public static LogRequest stopRequest() { | ||
| return new LogRequest(Type.STOP); | ||
| } | ||
|
|
||
| public static LogRequest pauseRequest() { |
There was a problem hiding this comment.
Actually looking closer, we do need the pause method for size. I will add this back in.
|
@wburns Now that I have got my head around the LogAppender changes LGTM, just a few minor things. As well as my comments there are a few more TODO statements that need addressing/removing. |
|
@tristantarrant given the fact that segmentation configuration parameters will be changed in this PR and the subsequent can you make a judgement if this requires waiting for a branch to be integrated into 13.0 only or if 12.1 is okay as well? |
|
Talking with Tristan he gave the green like for the changes into 12.1. However, we really need this PR and #9114 both integrated (granted the other relies upon this one). |
wburns
force-pushed
the
ISPN-12755_softindex_new_spi
branch
from
dd39d5b
to
83d4ae2
Compare
wburns
changed the title
wburns
force-pushed
the
ISPN-12755_softindex_new_spi
branch
from
83d4ae2
to
767e444
Compare
|
Great work @wburns |
https://issues.redhat.com/browse/ISPN-12755
This PR breaks segmented store configuration for soft index file store. The subsequent PR will add this support back in and segmented will be the only running operation as there is no need to not have segmented with the new algorithm.