-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add Windows native method to retrieve the number of allocated bytes on disk for file #79698
Changes from 9 commits
d13a95b
f4c5d77
8f95d7b
806f501
f109191
0ef20a0
39adb83
15abf62
d1c4f77
2af8a5a
2540f7e
ee5ab76
2d9275c
6773071
1cc458f
c6ba764
e5a0e2c
9258da1
e1aff78
c00295f
853a9de
5928b39
3357e8f
f77c0e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,11 @@ | |
import com.sun.jna.Native; | ||
import com.sun.jna.Pointer; | ||
import com.sun.jna.WString; | ||
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.apache.lucene.util.Constants; | ||
import org.elasticsearch.core.Nullable; | ||
import org.elasticsearch.monitor.jvm.JvmInfo; | ||
|
||
import java.nio.file.Path; | ||
|
@@ -260,4 +262,17 @@ static void tryInstallSystemCallFilter(Path tmpFile) { | |
} | ||
} | ||
|
||
/** | ||
* Returns the number of allocated bytes on disk for a given file. | ||
* | ||
* @param path the path o the file | ||
* @return the number of allocated bytes on disk for the file or {@code null} if the allocated size could not be returned | ||
*/ | ||
@Nullable | ||
static Long allocatedSizeInBytes(Path path) { | ||
if (Constants.WINDOWS) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the intent is to have this for stats, shouldn't we have a nix implementation as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I plan to add it in a follow up pull request |
||
return JNAKernel32Library.getInstance().allocatedSizeInBytes(path); | ||
} | ||
return null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,14 +10,15 @@ | |
|
||
import org.apache.logging.log4j.LogManager; | ||
import org.apache.logging.log4j.Logger; | ||
import org.elasticsearch.core.Nullable; | ||
|
||
import java.nio.file.Path; | ||
|
||
/** | ||
* The Natives class is a wrapper class that checks if the classes necessary for calling native methods are available on | ||
* startup. If they are not available, this class will avoid calling code that loads these classes. | ||
*/ | ||
final class Natives { | ||
public final class Natives { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about making this public. We've kept this package private because all this native stuff, thus far, is for Elasticsearch startup, hence the bootstrap package. If we want to start utilizing other native functionality for utilities like this, perhaps we should have eg in this case a filesystem utils class (outside of bootstrap, we could still force init early before SM is set). But I would like to know @ChrisHegarty 's thoughts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's the most problematic point of this change so I wanted to have your opinion. I initially wanted it to be in the searchable snapshot plugin but I failed to make it work (it is doable I think for Linux but definitely hard to do right for windows/kernel32). I like the idea of having outside of |
||
/** no instantiation */ | ||
private Natives() {} | ||
|
||
|
@@ -133,4 +134,17 @@ static boolean isSystemCallFilterInstalled() { | |
return JNANatives.LOCAL_SYSTEM_CALL_FILTER; | ||
} | ||
|
||
/** | ||
* Returns the number of allocated bytes on disk for a given file. | ||
* | ||
* @param path the path to the file | ||
* @return the number of allocated bytes on disk for the file or {@code null} if the allocated size could not be returned | ||
*/ | ||
@Nullable | ||
public static Long allocatedSizeInBytes(Path path) { | ||
if (JNA_AVAILABLE == false) { | ||
return null; | ||
} | ||
return JNANatives.allocatedSizeInBytes(path); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we define this in
test/framework
rather thanserver
since it's not used in prod code today? Or are we planning to use it in prod later on (e.g. expose sparse sizes in stats)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd like this to be returned as part of searchable snapshot's Cache Stats API, so that we have information about the shared cache and cold cache that sit altogether. What do you think?
I can try to make it work in the test framework; it should be OK as long as the static instance is created before the security manager is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: I'd like to have assertion in
CacheFile
itself that would retrieve the allocated size on disk and compares it with the completed ranges of a cache file, and verify if size vs ranges is not completly out of bounds.That would also require the method to be in
server
. I tried to make it work in the plugin itself but for JNA kernel32 library it's very difficult given all the permissions it requires (createClassLoader, setSecurityManager etc). Linux is OK though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 having these stats in future seems like a good plan, so I'm ok to leave the method where it is.