Skip to content
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

Added incremental id in history items #142

Conversation

SMorettini
Copy link
Contributor

Originating Project/Creator
Affected Component Event table

Change Description

Added an incremental ID to the objects stored in the HistoryHelper.

Rationale

The current implementation works only if there are no events with the same ID sent in the same timestamp. By adding the incremental-id is possible to have a unique ID for the events. This enables the support of events with the same ID sent in the same instant.

I know that sending an event with the same id at the same time is very improbable but it happened in my project. We were sending an ERROR in sequence and GDS crashed.

Copy link
Collaborator

@thomas-bc thomas-bc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look go to me, but I'm wondering about the motivation behind this.

We were sending an ERROR in sequence and GDS crashed.

Does that mean quite literally something like chaining two log_[...]_MYEVENT() function calls together? What is prompting this design?

@SMorettini
Copy link
Contributor Author

Does that mean quite literally something like chaining two log_[...]_MYEVENT() function calls together? What is prompting this design?

In my code, I had a loop to check for some parameters and send an error for each parameter out of a certain limit. The event was something like The parameter {id_param} was above the limit. I used the same event for all the parameters but just changed the id_param and in some rare cases, it happened that I sent those events at the same timestamp. I'll probably change the code to send less recent but still I think GDS should not crash.

If you want a more detailed example I can try to write down a small example for it.

@thomas-bc
Copy link
Collaborator

Ohh ok understood no that makes sense thanks!

@LeStarch
Copy link
Collaborator

@SMorettini I made the change here to switch to a basic counter to get incremental ids as this is, I think, less likely to collide due to reused indexes.

We really hope we aren't stepping on your toes touching up your PRs. We love the work and do not wish to over-burden you with many requests and small changes. We are also happy to hear your feedback on any of our suggestions/fixes.

On this one, we would like your help to verify the fix, as I cannot seem to duplicate the original behavior. Hints on how to duplicate it, or testing out the fix would be much appreciated. I tried many back-to-back event calls, but that did not seem to cause collisions.

Thanks for all you've contributed!

@thomas-bc thomas-bc merged commit c9ac315 into nasa:devel Oct 24, 2023
12 checks passed
@SMorettini SMorettini deleted the Add-visualization-suppoprt-to-event-with-same-id-and-same-time branch November 16, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants