moved max size check; added test for post-max size functioning#1130
moved max size check; added test for post-max size functioning#1130mattsains wants to merge 5 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mattsains The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
| // however, I don't quite understand it. Why is the allocation increment added to the size required, | ||
| // rather than the size required rounded up to the next multiple of the allocation increment? |
There was a problem hiding this comment.
It's over allocation, to amortize the cost of truncate and fsync
refer to a122e1c
There was a problem hiding this comment.
My issue was mostly that what I think happens here is that if you ask for 1 byte, but the alloc size is 10 bytes, it actually allocates 11 bytes. Shouldn't it be 10?
There was a problem hiding this comment.
yes, as mentioned above it's a bit over allocation. By default it's 16MB at most. It shouldn't be a big problem. The logic has been there for almost a decade, let's keep it as it's unless you have a very strong justification.
Co-authored-by: Benjamin Wang <benjamin.wang@broadcom.com> Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
|
@ahrtr I'm going to have to add special logic for windows because on that platform, to open a file with an mmap size larger than the file, you have to expand the file |
I don't think we need a special logic for windows. I don't see an issue. If you see an issue, then pls add a test to reproduce/prove it first. |
| // that is beyond the maximum size does not grow the db on Windows. | ||
| // In Windows, the file must be expanded to the mmap initial size. | ||
| // https://github.com/etcd-io/bbolt/issues/928 | ||
| func TestDB_MaxSizeExceededDoesNotGrow(t *testing.T) { |
There was a problem hiding this comment.
@ahrtr I've updated this test to demonstrate the issue on Windows I was talking about. When you set InitialMmapSize to be greater than the actual size of the file, the file must be expanded to map it that way.
The assertion on line 1666 fails because newSize is exactly double expandedSize (or, exactly equal to InitialMmapSize)
There was a problem hiding this comment.
I see.
I think we need to disable InitialMmapSize for Windows; in other words, we don't use it for Windows.
Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
|
are you still working on this PR? |
sorry, I am having trouble prioritising this issue. If anyone else can do it, go ahead, otherwise I'll get to it when I can |
…izeReached Fixes etcd-io#1108. Squashed from etcd-io#1130: * db4544f moved max size check; added test for post-max size functioning * 9445cc1 Apply suggestions from code review * 2a8037d merge fillDBWithKeys * 60ee0ee add test demonstrating windows issue * e4590be update comment for test Signed-off-by: Matthew Sainsbury <msainsbury@microsoft.com>
…izeReached Fixes etcd-io#1108. Squashed from etcd-io#1130: * db4544f moved max size check; added test for post-max size functioning * 9445cc1 Apply suggestions from code review * 2a8037d merge fillDBWithKeys * 60ee0ee add test demonstrating windows issue * e4590be update comment for test Co-authored-by: Matthew Sainsbury <msainsbury@microsoft.com> Signed-off-by: elbehery <elbeherymustafa@gmail.com>
3 participants
Fixes #1108