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

PR - Issue 49602 - Revise replication status messages #3505

Closed
389-ds-bot opened this issue Sep 13, 2020 · 17 comments
Closed

PR - Issue 49602 - Revise replication status messages #3505

389-ds-bot opened this issue Sep 13, 2020 · 17 comments
Labels
merged Migration flag - PR pr Migration flag - PR

Comments

@389-ds-bot
Copy link

389-ds-bot commented Sep 13, 2020

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50447


Bug Description: All agreement status messages start with "Error (##)" followed
by a text string. Even success states start with "Error", and
this is confusing.

             Instead use new keywords:  Good, Warning, Bad

             Also add new attributers to display the status in a JSON format
             for easier parsing for applications:

                 replicaLastUpdateStatusJSON
                 replicaLastInitStatusJSON

Design Doc: https://www.port389.org/docs/389ds/design/repl-agmt-status-design.html

Resolves: #2661

@389-ds-bot 389-ds-bot added merged Migration flag - PR pr Migration flag - PR labels Sep 13, 2020
@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 09:46:08

Minor nitpick, if you have time can we use snprintf rather than PR_snprintf? No issue if you don't want to change.

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 09:46:38

As discussed, 8601 timestamps may be better (I think access log does them?)

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 09:47:08

And finally, good maybe better as "operational" or "green" etc. Email has more. Otherwise looks good!

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 10:03:31

Ahh one more point - when we update the json field moving between healthy -> warning/error, we should write to the error log at the same time, and if we move from warning/error -> healthy, we should log that too (but subsequent "healthy" messages shuold not be logged ...).

This could be a future addition though, but it would help gss diagnose issues in retrospect rather than relying on monitoring the attribute.

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 15:43:01

As discussed, 8601 timestamps may be better (I think access log does them?)

I added the ISO 8601 timestamp already - is this not what you wanted? I'm confused

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 15:48:01

And finally, good maybe better as "operational" or "green" etc. Email has more. Otherwise looks good!

This is getting silly about the names. Perhaps this ticket should be pushed out to 1.4.2 as its going to miss the deadline anyway (today) --> which means once again we missed ALL the rfe's for the next release.

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 16:03:32

I did just say pick one and be done ... :(

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 16:03:49

As discussed, 8601 timestamps may be better (I think access log does them?)

I added the ISO 8601 timestamp already - is this not what you wanted? I'm confused

Maybe I misinterpreted this becausu in your email you said you wourd remove them?

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 16:32:19

As discussed, 8601 timestamps may be better (I think access log does them?)
I added the ISO 8601 timestamp already - is this not what you wanted? I'm confused

Maybe I misinterpreted this becausu in your email you said you wourd remove them?

Well I looked into it and it was trivial - so I added it and updated the design doc.

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 17:19:31

I'm really sorry about this. I should have check the code more carefully, and I'm really sorry for the miscommunication. I'd say in the future I should do better in my review to check this, we should have kept the communication to a single source (maybe just the pr), and that it's also a good idea to comment the code to address comments that were made in the code.

Sorry this was delayed, :(

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 17:22:03

rebased onto de38d2eb4bd1cdc2ca9cfd09bd7e8cb8a23c9c85

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 17:24:09

rebased onto 35f5d6db6517bc8a64f34c994b21888f5f3254dc

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 17:25:25

rebased onto b4b1df6eb8ae12c0c8ca091fdc44d63d65998eab

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2019-06-14 17:35:03

ack - thanks so much for your patience

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 20:33:09

rebased onto bd80a4f

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2019-06-14 20:33:56

Pull-Request has been merged by mreynolds389

@389-ds-bot
Copy link
Author

Patch
50447.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged Migration flag - PR pr Migration flag - PR
Projects
None yet
Development

No branches or pull requests

1 participant