fstore: hold more than 64 files #15
Conversation
|
|
||
| evict = files_up_add(fs, fsf); | ||
| if (evict != NULL) { | ||
| ret = cio_chunk_down(evict->chunk); |
There was a problem hiding this comment.
Add comment do describe unmapping
| @@ -40,6 +40,7 @@ struct flb_fstore_file { | |||
| struct cio_chunk *chunk; /* chunk context */ | |||
| struct cio_stream *stream; /* parent stream that owns this file */ | |||
| struct mk_list _head; /* link to parent flb_fstore->files */ | |||
| struct mk_list _cache_head; /* link to flb_fstore->files_up cache */ | |||
There was a problem hiding this comment.
should we use the variable to _files_up_head?
There was a problem hiding this comment.
I personally like _cache_head to remind that the _files_up list is used as a cache of up files, but can switch to _files_up_head.
| _new->next = next; | ||
| _new->prev = prev; | ||
| prev->next = _new; | ||
| next->prev = _new; /* how is this a bug not found? is it really? */ |
There was a problem hiding this comment.
as discussed, you can move this after row 78.
There was a problem hiding this comment.
I forgot to change the corresponding PR on Monkey Server... So the solution that got accepted has the weird ordering... Shucks. At least it fixes the bug.
| @@ -179,7 +179,7 @@ int s3_store_buffer_put(struct flb_s3 *ctx, struct s3_file *s3_file, | |||
| } | |||
|
|
|||
| /* Append data to the target file */ | |||
| ret = flb_fstore_file_append(fsf, data, bytes); | |||
| ret = flb_fstore_file_append(ctx->fs, fsf, data, bytes); | |||
There was a problem hiding this comment.
fsf used to be used as se_file->fsf. Why is the reason for using cts->fs directly here?
There was a problem hiding this comment.
fs which is the file store context has the up list which needs to be updated on append. fs is found in the s3 context. The function signature needed to be altered accordingly.
| * | ||
| * max of max_chunks_up files allowed before eviction of least recently added file | ||
| */ | ||
| static struct flb_fstore_file *files_up_add(struct flb_fstore *fs, |
There was a problem hiding this comment.
please comments on what is the expected return output and its meaning.
| } | ||
|
|
||
| /* lets down file if needed, fsf must be brought up soon after by caller */ | ||
| static int file_up_prep(struct flb_fstore *fs, /* previously files_up_make_room */ |
There was a problem hiding this comment.
can we put more comments on what this function does and the meaning of return value?
There was a problem hiding this comment.
Sure. Compared to Eduardo's functions my functions already have a lot of comments. Disregarding conformity, this should be fine.
| * | ||
| * files_up record is reordered to put recently checked files first. | ||
| */ | ||
| static int file_up_if_down(struct flb_fstore *fs, |
There was a problem hiding this comment.
IMO _up_ should be used preferably to _mapped_, because we are interfacing with the mmap syscall through chunkio api. The chunkio concept of up for files is mmap, but it is probably a good idea for the code to follow chunkio's concept abstraction
|
Unfortunately I made this branch to be immutable, since it is cherry picked from, so I'll have to update the code to resolve these comments on this branch/pr: #16 |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Documentation
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.