add onDiskSize to core status API #223
Conversation
| if (directory instanceof OnDiskSizeDirectory) { | ||
| return onDiskSizeOfDirectory(directory); | ||
| } | ||
| return sizeOfDirectory(directory); |
There was a problem hiding this comment.
Note: at the Directory level it totally makes sense to fallback to sizeOfDirectory() here. This is a bit counterintuitive considering that falling back was the source of the "drift" bug, but it's different at the Directory level:
Any Directory impl that makes no distinction between "onDisk" and "logical" does so because it's writing everything to disk. It also makes sense to continue to refer to this as "onDisk" as opposed to "compressed" or whatever, because compression is orthogonal. Also, the fact that the access copy in our case is also technically "on disk" -- albeit an ephemeral local ssd -- is an implementation detail. For semantic purposes the access copy should be considered a cache.
| NamedList<Object> nodeStatus = new SimpleOrderedMap<>(); | ||
| if (it.handler.coreContainer != null) { | ||
| Path solrDataHome = it.handler.coreContainer.getCoreRootDirectory(); | ||
| File dataDir = solrDataHome.toFile(); | ||
| nodeStatus.add("indexDiskSizeTotal", dataDir.getTotalSpace()); | ||
| nodeStatus.add("indexDiskSizeAvailable", dataDir.getUsableSpace()); | ||
| } | ||
| it.rsp.add("nodeStatus", nodeStatus); |
There was a problem hiding this comment.
tbh I'd be fine leaving this, if you like. Either way.
This might even be upstreamable. Serves other directory impls, and gives data even when indexInfo=false (more efficient).
|
|
||
| private static class Sizes implements Accountable { | ||
| private static final long RAM_BYTES_USED = | ||
| RamUsageEstimator.shallowSizeOfInstance(SizeAccountingIndexOutput.class); |
There was a problem hiding this comment.
This should instead be RamUsageEstimator.shallowSizeOfInstance(SizeAccountingIndexOutput.Sizes.class), no?
| // backing IndexOutput does not allow us to get onDiskSize | ||
| return 0; |
There was a problem hiding this comment.
gah -- this doesn't matter for us practically, but I think we may need a conditional here. We should return 0 iff backing IndexOutput instanceof SizeReportingIndexOutput AND backing directory instanceof OnDiskSizeDirectory. Otherwise, in keeping with how non-OnDiskSizeDirectory onDiskSize is handled, we should indeed fall back to returning sizeOf (as before).
| return ((DirectoryFactory.OnDiskSizeDirectory) in).onDiskFileLength(name); | ||
| } else { | ||
| // directory does not implement onDiskSize | ||
| return 0; |
There was a problem hiding this comment.
here too I suspect that for consistency we should actually fallback to return simple sizeOf().
| long onDiskSize = this.getFilePointer(); | ||
| if (backing instanceof CompressingDirectory.SizeReportingIndexOutput) { | ||
| onDiskSize = ((CompressingDirectory.SizeReportingIndexOutput) backing).getBytesWritten(); | ||
| } |
There was a problem hiding this comment.
This is the same issue as the accounting drift, but it only happens on init, so it's harder to notice. I think we want to do the same here as we compute onDiskSize:
if (backing instanceof SizeReportingIndexOutput)
onDiskSize = getBytesWritten()
else if (backingDirectory instanceof OnDiskSizeDirectory)
onDiskSize = 0
else
onDiskSize = getFilePointer()
| long onDiskSize = 0; | ||
| if (backing instanceof CompressingDirectory.SizeReportingIndexOutput) { | ||
| long finalBytesWritten = getBytesWritten(backing); | ||
| onDiskSize = finalBytesWritten; | ||
| // logical size should already be set through writeByte(s), but we need to finalize the | ||
| // on-disk size here | ||
| sizeWriter.apply(0, finalBytesWritten - lastBytesWritten, name); | ||
| } | ||
| fileSizeMap.put(name, new Sizes(backing.getFilePointer(), onDiskSize)); |
There was a problem hiding this comment.
here too I think we want a 3-way condition, as above
if (backing instanceof SizeReportingIndexOutput)
onDiskSize = getBytesWritten()
else if (backingDirectory instanceof OnDiskSizeDirectory)
onDiskSize = 0
else
onDiskSize = getFilePointer()
| if (secondary instanceof CompressingDirectory.SizeReportingIndexOutput) { | ||
| return ((CompressingDirectory.SizeReportingIndexOutput) secondary).getBytesWritten(); | ||
| } else { | ||
| return getFilePointer(); | ||
| } |
There was a problem hiding this comment.
I think this instanceof fallback is ok, as far as semantics of getBytesWritten()? wdyt?
|
@magibney I've updated this PR with all of your suggestions. It passes unit tests and my local integration test. Mind taking another look? |
* add onDiskSize to core status API * tidy * add back indexDiskSize * fix typo in Sizes RamUsageEstimator * add some needed conditionals to onDiskSize calcs * tidy * add backingDirectory to SizeAccountingIndexInput * tidy
* add onDiskSize to core status API (#223) * add onDiskSize to core status API * tidy * add back indexDiskSize * fix typo in Sizes RamUsageEstimator * add some needed conditionals to onDiskSize calcs * tidy * add backingDirectory to SizeAccountingIndexInput * tidy * tidy
rebase of #212