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

error: warning array concept added #2685

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Piankero
Copy link
Contributor

commented May 9, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@Piankero Piankero requested a review from markus2330 May 9, 2019

@markus2330
Copy link
Contributor

left a comment

Thank you.


Currently multiple warnings are saved in an elektra non-conforming array
notation which is limited to 100 entries. The notation of `#00` is against
the design [decision made](array.md).

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor
Suggested change
the design [decision made](array.md).
a [design decision](array.md).
notation which is limited to 100 entries. The notation of `#00` is against
the design [decision made](array.md).

Furthermore if the number of warnings is exceeded, the earliest entries are overwritten

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

This is not a problem?

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

We discussed this with @kodebach in an elektra meeting that this is a problem.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

The problem are situations like this:

#01: the newest warning
#02: the oldest warning
...
#99:  the second newest warning

So that new/old warnings might get mixed up.

There are several ways how to avoid it, one way is to simply continue counting up and start dropping the warnings with the lowest index.

I remember that @kodebach said that we should also add a warning when we started doing something like this.

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

How can this happen?
What do you mean by "dropping the warnings with the lowest index"?
Should it be possible to have only entries from #3 to #99 for example because 0-2 is dropped?

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Exactly. (#99 would be #_99, though)

This comment has been minimized.

Copy link
@kodebach

kodebach May 13, 2019

Contributor

As far as I remember we agreed on this:

Handle the first n-1 warnings as normal. Instead of the n-th warning we issue a special warning that the limit has been reached and ignore the actual warning. From then on all warnings are dropped. Additionally we could simply log all warnings above the limit, so that in case of debugging the warnings are not completely gone.

The reason for this behaviour is that often the first warning created is the most important to fix the problem. Later warnings may be caused by earlier ones, if the earlier warning is overwritten it becomes hard to diagnose.

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

Exactly. (#99 would be #_99, though)

Why exactly the underscore? This is really confusing.
I am also confused why we should only partially drop entries. What I have seen in the code is that you iterate over an elektra array and also save the current array number under warnings/number.

Instead of the n-th warning

What exactly is n here? @markus2330 does not want a hard cap but this obviously requires one.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Additionally we could simply log all warnings above the limit, so that in case of debugging the warnings are not completely gone.

In debugging you usually only have short sessions (not many kdbGet in a long-running loop) thus it is rather unlikely you will reach the limit.

The reason for this behaviour is that often the first warning created is the most important to fix the problem.

This is true for warnings within a single execution of kdb*-call (e.g. kdbSet) In applications many warnings might get collected until finally an error occurs and everything is presented to the user. Then the most recent warnings are relevant. Not what happened a long time ago.

Alternatively, we could simply clean all warnings before a kdb*-call. Then the behavior as described by @kodebach is best (only keep the earliest warnings). This decision does not mention this, however.

Later warnings may be caused by earlier ones, if the earlier warning is overwritten it becomes hard to diagnose.

Yes, within a single kdb*-call it is like this.

Why exactly the underscore?

It is needed for correct sorting.

This is really confusing.

You are free to implement #2698. It would be highly appreciated.

This comment has been minimized.

Copy link
@Piankero

Piankero May 16, 2019

Author Contributor

I am much more confused now than before. I still don't know why you simply drop complete warnings when the current warning number is saved in the parent meta (warning/number). When and how to use these weird underscores? Is there any document which explains this? I will stick to the array number of 100 because there is neither better a reason to use 1000 nor to use 10.
What exactly is the desired behavior now when n is exceeded?
I feel like consuming more time from you rather if someone else implements this with more insight in Elektra ...

This comment has been minimized.

Copy link
@kodebach

kodebach May 16, 2019

Contributor

When and how to use these weird underscores?

Simply put: Elektra sorts lexicographically using ASCII codes. _ has a higher ASCII code than the digits 0-9. Therefore by prefixing the index with _ we ensure that the sort order is #0, #1, #2, ..., #9, #_10 instead of #0, #1, #10, #2, ..., #9. You need more underscores, the longer the number is because otherwise the same problem occurs for 10 and 100 instead of 1 and 10.

Is there any document which explains this?

Yes, https://github.com/ElektraInitiative/libelektra/blob/master/doc/tutorials/arrays.md. Feel free to improve it, if it isn't clear enough.

What exactly is the desired behavior now when n is exceeded?

For now, I think we should stick to the old behaviour of overwriting old warnings (so basically the index is always modulo n), because of the problem with multiple kdb*-calls.


The format should be aligned with the correct array notation,
starting with `#0`. The maximum number of warnings will be expanded to
1001 entries (`#0` - `#1000`).

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Actually, there is no reason to change the current value of 100, nobody complained that it is too low. It only should not be hard-coded that it must be 100.

Suggested change
1001 entries (`#0` - `#1000`).
1001 entries (`#0` - `#___1000`).

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

We agreed in the elektra meeting to hardcap the limit in case of recursive functions which issue warnings and never end.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Yes, we agreed on that. But not on a concrete number.

If you give any reason for why it should be 1000, it might be ok.

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

Actually I remember that we agreed on the number 1000 but any other number is fine too. But I think we need a hard-cap somewhere to avoid filling up the whole system with warning messages if someone accidentally introduces an endless loop.

starting with `#0`. The maximum number of warnings will be expanded to
1001 entries (`#0` - `#1000`).

If more warnings occur, warning `#1000` is overwritten each time.

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

This might be a problem as one kdbGet/kdbSet sometimes yield more than one warning. Isn't it better to drop old warnings?


## Rationale

## Implications

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Missing.


If more warnings occur, warning `#1000` is overwritten each time.

## Rationale

This comment has been minimized.

Copy link
@markus2330

markus2330 May 13, 2019

Contributor

Missing. (To confirm to the array notation.)

Is there any reason why it should be 1001?

This comment has been minimized.

Copy link
@Piankero

Piankero May 13, 2019

Author Contributor

1001 is just because it is more natural (to me) to run from 0-1000 instead of 0-999. I have no problem to limit it somewhere else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.