Overhaul FASTER page and segment size parameters #230
Conversation
… for hybrid log segment size
| LogCommitManager = this.UseLocalFiles | ||
| ? null // TODO: fix this: new LocalLogCommitManager($"{this.LocalDirectoryPath}\\{this.PartitionFolderName}\\{CommitBlobName}") | ||
| : (ILogCommitManager)this, |
There was a problem hiding this comment.
can you please elaborate on this TODO comment? What needs fixing exactly?
There was a problem hiding this comment.
right, this was just a copy-paste (so it has nothing to do with this PR). The TODO here refers to the fact that at some point Netherite supported using the file system (instead of Azure Storage) to store task hubs. But it is not an official feature and commented out at the moment. There is some scenario in which we bring this back on K8s, therefore I did not want to just delete it outright.
There was a problem hiding this comment.
ah gotcha. That makes sense.
Might be helpful just to add a blurb to that comment that this local-files support is for K8s but not advertised today.
| public static (int pageSizeBits, int segmentSizeBits) GetImmutableStoreLogParameters(bool useSeparatePageBlobStorage, FasterTuningParameters tuningParameters) | ||
| { | ||
| int pageSizeBits = tuningParameters?.StoreLogPageSizeBits ?? 10; // 1kB | ||
| int segmentSizeBits = tuningParameters?.StoreLogSegmentSizeBits ?? 19; // 512 kB | ||
|
|
||
| return (pageSizeBits, segmentSizeBits); |
There was a problem hiding this comment.
It appears the first parameter is not being used here, or is it? Can we remove it?
There was a problem hiding this comment.
I will remove it. It was used in a previous version.
| @@ -157,10 +174,21 @@ public class FasterTuningParameters | |||
|
|
|||
| public static string GetStorageFormat(NetheriteOrchestrationServiceSettings settings) | |||
There was a problem hiding this comment.
I noticed this doesn't seem to be called in this PR. Is it called elsewhere?
There was a problem hiding this comment.
Yes, it is called when a new taskhub is created in IStorageLayer.CreateTaskhubIfNotExistsAsync. At that point, all the important parameters for this task hub storage format are recorded inside taskhubparameters.json.
| @@ -201,6 +232,28 @@ public static void CheckStorageFormat(string format, NetheriteOrchestrationServi | |||
| { | |||
| throw new NetheriteConfigurationException($"The current storage format version (={StorageFormatVersion.Last()}) is incompatible with the existing taskhub (={taskhubFormat.FormatVersion})."); | |||
| } | |||
|
|
|||
| // read the immutable log parameters from storage, and keep them as a tuning parameter | |||
| // (which may override any tuning parameters set by the user; that is what we want since they cannot be honored) | |||
There was a problem hiding this comment.
can we throw a warning if we detect this conflict: that the new parameters cannot be honored?
There was a problem hiding this comment.
That is a good idea. I think we used to have a warning like that for the partition count (which is also an immutable parameter) but I can't find it anymore in the code. It should be there also.
| public static void CheckStorageFormat(string format, NetheriteOrchestrationServiceSettings settings) | ||
| public static void CheckAndLoadStorageFormat(string format, NetheriteOrchestrationServiceSettings settings) |
There was a problem hiding this comment.
super nitpick: at first glance, I find it a bit unintuitive to first check and then load the config here. The opposite order makes more sense to me, as it allows us to validate the config we just loaded. I realize this current ordering works because we don't really throw any validation errors for the config we load.
I'll make a soft suggestion to rename this to LoadAndCheckStorageFormat and check the order of operations to match that new name, which I think is more future-proof. But feel free to disregard this comment as it's mostly a stylistic nit in the present.
There was a problem hiding this comment.
I have reworked this a bit to make it clearer.
…ed even if using client only)
The following FASTER tuning parameters are immutable, i.e. must not be changed after a task hub is created (or else the storage provider cannot start any more):
However, sometimes we do need to change these parameters. Most recently (#229), we found that object logs are not collected aggressively enough after compaction, because they hybrid log segment size is much too large. Therefore, we want to change this default.
In this PR, we
taskhubparameters.jsonfile of the task hub, so that we can restore them when loading a task hub, which guarantees that parameters always match what is in storage even if the defaults change or the user explicitly changes them.For compatibility, if loading a task hub that has no sizes recorded, we assume it uses the default sizes (as of the time of this PR).