add notification rotation file

[ntadas]

Overlook of the changes:

Add to the struct stream 2 new fields, a bool stating if its the current/latest stream and a pointer to the stream to rotate to. Add to the offset struct the event_fd of the current stream being processed

The main idea is behind it is to rotate between the original stream, lets say NETCONF.event file and a rotation stream, NETCONF.event.rotate.

When the server starts it writes both files (normal and rotation one) and makes sure that the rotation stream is connected to the "original" stream.

The ncntf_event_store function checks if the new event will overlap the max size define for the stream, if so it will write a end marker to the file and write the event in the next stream (also updates the current stream marker).

The ncntf_stream_iter_next starts from the oldest stream and when it detects the end marker, jumps to the rotation stream. additionally it uses the off_struct to know what stream is being consumed.

Currently the limit is 0,5MB so that its easy to test, this number can be high and we can even give the option to configure this with a compilation flag.

[rkrejci]

I have some questions/issues:

1. when it starts, the events files are opened by read_fileheader(), but I don't see where the .current flag for one of the rotating files is set. In which of them the events will be stored?
2. when replaying events, the reading should start in the older of the rotating files, but it starts (ncntf_stream_iter_start()) in the .current stream file so the events from the other file are hidden
3. when freeing stream structure (ncntf_stream_free()) it seems to possibly get into infinite loop (and double free) by calling another ncntf_stream_free() on .next_rotate, since its .next_rotate will be again the first stream structure, right?

[ntadas]

you are right in all 3 points. I've just fixed point 1 and 3 but still need to think how to solve point 2.

[ntadas]

now I'm starting in the old rotating file unless its not a replay subscription

[ntadas]

with this last fix, do you think all 3 points are addressed?

[rkrejci]

Unfortunatelly, it still doesn't work.

• please init variable s2 (it's not a bug, but compiler complies to it):

@@ -572,7 +572,7 @@ static struct stream *read_fileheader(const char* filepath, bool second)
 {
        struct stream *s;
        char* rotate_filepath = NULL;
-       struct stream *s2;
+       struct stream *s2 = NULL;
        int fd;
        char magic_name[strlen(MAGIC_NAME)];
        uint16_t magic_number;

• it seems that if replay is not requested (in netopeer-cli, I don't use --begin -X), it works fine. But requesting replay is broken and only the last message is sent from server (and no replayComplete)

netconf> subscribe --begin -500

Just a note - for better testing of changing between the event files, I have changed NCNTF_STREAMS_MAX_SIZE to 1024, maybe this can cause that you did not catch this during testing the feature.

[ntadas]

Hi,

due to the issues found, I've decided to go in a different way. Instead of having 2 files to keep it simple I've reworked the code to have only one file and rotate over it.

• add EOF marker when max file len is reached.
• add END marker when writing a notification to the stream, so that we always know what is the last notification.
• stop reading the stream when the END marker is found (storing the offset to easy the code logic)
• in the iter_next jump to the beginning of the notification stream when the EOF marker is found
• check if data is available to read by checking if the cur_off is diff from the END marker offset.

the main limitation with this approach is that we can only replay from the beginning of the file, until the END marker. this means that if the file was never rotated we have the old behaviour, but once we have the rotation, we will lose all the notifications starting from END marker to EOF marker.

this small limitation can also be reworked later (e.g. check if we have data after the END marker and start pointing to the first offset after the END marker instead of the beginning of the data).

[rkrejci]

Hi, I'm going to check the code, but I have one idea about the improvement of the limitation you are describing. What about a special value of NCNTF_STREAMS_MAX_SIZE (maybe -1) meaning that no rotations is done. So, such a value would preserve the current behavior. I believe that it is not a big change to your code, but it allows to avoid the replay limitation if it is not acceptable. What do you think?

And one note about formatting - you are mixing if() with if () and if () { with if ()\n{. Please, use if () { format.

[ntadas]

Regarding the if formatting I'll rework it to be complaint with the libnetconf code. About the comment to have the possibility of -1, I agree.

[rkrejci]

ok, it seems fine now. So I will wait for that no-rotate feature (maybe 0 as a special value would be better) and then merge it.

[ntadas]

Just did the commit, can you please check if everything is ok. I've added the configure option --with-maxnotificationfilesize where we can specify the notifications file size. if this option is not used we have the file growing as before this fix/feature.

[rkrejci]

Seems working fine, thanks!

[rkrejci]

I have squashed the commits and merged it into master, so despite it is not visible here, the pull request is merged as c535d8be

[ntadas]

thanks a lot.

On Wed, Sep 21, 2016 at 1:56 PM, Radek Krejčí notifications@github.com wrote:

I have squashed the commits and merged it into master, so despite it is not visible here, the pull request is merged as c535d8b https://github.com/CESNET/libnetconf/commit/c535d8bed01a1309e1645f11c6ca786b3b98dac7

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/CESNET/libnetconf/pull/208#issuecomment-248603936, or mute the thread https://github.com/notifications/unsubscribe-auth/APo_hvzmCdJAQMaFhKHts4gOn91Z3cWbks5qsSmVgaJpZM4JFKnC .