CA-420987: xapi_message: Add message-destroy-all API#6806
CA-420987: xapi_message: Add message-destroy-all API#6806cplaursen wants to merge 3 commits intoxapi-project:masterfrom
Conversation
930ef54 to
4e1c6da
Compare
|
Could we have an alternative approach where messages are deleted depending on a filter? In particular by age and by priority are interesting here. |
|
If you would like to over-engineer this:
exception Failure of string
type expression
type value = Bool of bool | Float of float
type env
val empty : unit -> env
val add : env -> string -> float -> unit
val add' : env -> string list -> float -> unit
val env : (string * float) list -> env
val compile : string -> expression
val string : env -> string -> value
val expr : env -> expression -> value
val simple : string -> valueYou can provide an environment that binds names to values and an expression which then can be evaluated: $ make utop
utop # Expr.Eval.simple "3+4*5 == 23";;
- : Expr.Eval.value = Expr.Eval.Bool true
utop # let env = Expr.Eval.env ["pi", Float.pi] in
Expr.Eval.string env "pi * pi";;
- : Expr.Eval.value = Expr.Eval.Float 9.86960440108935799If you want to do this, would suggest to just copy the |
4e1c6da to
4cbe39f
Compare
|
@psafont I've added parameters to select messages to delete by priority, before a timestamp, and after a timestamp. Is this what you were hoping for? |
|
@lindig I'll keep it simple for now, I don't see a need for advanced filtering though it would be a fun thing to do. If the frontend people need it I can always add it with an extra filter= parameter. |
psafont
left a comment
There was a problem hiding this comment.
The current PR only modifies the CLI interface (xe is its only user), and doesn't modify the API for Xencenter, is this intended?
To modify the API, the functions are in ocaml/idl and, maybe https://github.com/xapi-project/xen-api/blob/master/ocaml/xapi/xapi_message.ml as well
4cbe39f to
dfaec8f
Compare
|
Thanks for that, I've added the new message to idl and moved the implementation over to xapi_message |
627b221 to
fb618b2
Compare
|
Just tested it and the command works as expected. |
| ; param_default= Some (VInt (-1L)) | ||
| } | ||
| ] | ||
| ~allowed_roles:_R_POOL_OP () |
There was a problem hiding this comment.
I think with the params, the API is not convenient to use.
I have tested the python SDK:
session.xenapi.message.destroy_all()
session.xenapi.message.destroy_all('20251222T00:00:00Z', '20251222T00:00:00Z', 1)
are OK.
session.xenapi.message.destroy_all(1)
session.xenapi.message.destroy_all(priority=1)
failed.
named param is not supported, then the user has to provide all the three params even if he may just want to filter the priority.
Would you consider a new API Seems where expr is not suitable either. I have no better idea about this.destroy_all_where, referring to message.get_all_records_where, while the destroy_all will not filter any conditions.
There was a problem hiding this comment.
I believe the documentation should say explicitly if conditions are logically and or or: have all conditions to be met or just at least one? What happens of no conditions are provided?
psafont
left a comment
There was a problem hiding this comment.
Only a minor comment about docs and some unexpected changes
ocaml/idl/datamodel.ml
Outdated
| ; { | ||
| param_type= Int | ||
| ; param_name= "priority" | ||
| ; param_doc= "Priority of messages to be destroyed" |
There was a problem hiding this comment.
Is there a place where the values of different priorities are explained? It would be useful here
There was a problem hiding this comment.
Would a different API be more convenient?
- an single timestamp
- a direction: before/after
with the semantics that all messages before/after the timestamp are removed. I would imagine that this is the common use case.
d93fd06 to
7a0cb70
Compare
ocaml/idl/datamodel.ml
Outdated
| ; { | ||
| param_type= Int | ||
| ; param_name= "priority" | ||
| ; param_doc= "Priority of messages to be destroyed" |
There was a problem hiding this comment.
Would a different API be more convenient?
- an single timestamp
- a direction: before/after
with the semantics that all messages before/after the timestamp are removed. I would imagine that this is the common use case.
We've seen issues where a client will try to dismiss all their alerts and fail due to the message-destroy call hitting the maximum XAPI request size at ~5000 messages. A new message-destroy-all API avoids this issue while saving the bandwidth required to send a ref for each message. Users of the new API call can choose to dismiss messages that arrived before or after a certain date, or only those with a given priority. Signed-off-by: Christian Pardillo Laursen <christian.pardillolaursen@citrix.com>
1bb1f73 to
5289c27
Compare
|
The destroy_all call now accepts an assoc list of optional parameters: priority, before, and after. These are optional parameters of the CLI call. Still need to test but all being well I think this should be fine to merge. |
… call Signed-off-by: Christian Pardillo Laursen <christian.pardillolaursen@citrix.com>
5289c27 to
b80fb5a
Compare
| ; param_default= Some (VInt (-1L)) | ||
| } | ||
| ] | ||
| ~allowed_roles:_R_POOL_OP () |
There was a problem hiding this comment.
I believe the documentation should say explicitly if conditions are logically and or or: have all conditions to be met or just at least one? What happens of no conditions are provided?
Signed-off-by: Christian Pardillo Laursen <christian.pardillolaursen@citrix.com>
We've seen issues where a client will try to dismiss all their alerts and fail due to the message-destroy call hitting the maximum XAPI request size at ~5000 messages.
A new message-destroy-all API avoids this issue while saving the bandwidth required to send a ref for each message.