Skip to content

Conversation

@Barthelemy
Copy link
Collaborator

We have encountered cases when the users would send very large objects to the CCDB and causing havoc later on. We would like to limit the size of the objects sent to the QCDB.

I propose to add a parameter to the store* methods specifying such a maximum limit.

Doing it on the client side, i.e. in the QC, would mean that we have to do the gymnastic of serializing the object just to know its size although it is done any ways in the CcdbApi. Therfore, I prefer to do it in the ccdbapi to avoid doing it twice in particular given that it is a very expensive operation.

I also considered to have the max size as an object member. However one might want to vary the max size of the objects.

Last note: the logging of the message when an object is too big might be an issue as it could be repeated many times. On the other hand it is a debug message and it is quite important to be informed that the operation failed. I considered throwing an exception but it would change the interface of these methods and I was not sure it would be wise.

What do you think ?

@Barthelemy Barthelemy requested review from a team, costing and sawenzel as code owners January 26, 2022 13:48
@ktf
Copy link
Member

ktf commented Jan 26, 2022

Shouldn't this be enforced on the server side? @costing

@costing
Copy link
Collaborator

costing commented Jan 26, 2022

I think it would be more efficient to not send it at all than to be rejected after the transfer. But yes, the server could also impose them. Just that a client could have a much finer grained policy on what is acceptable or not in each situation.

The question on how to signal this to the client applies in both cases. What if instead of an exception the return values of these methods would change from void to int, with a0 for success and various exit codes internal (object too big == 1...) or simply the HTTP code if the request failed. Would that be a too big API change?

@Barthelemy
Copy link
Collaborator Author

@ktf @costing A limit on the server-side is a last resort safety. I think that it should also be implemented. Good idea @ktf. Nevertheless, we need a finer grain system because different tasks have different needs. In the QC, we also consider having different size for different data types.

Concerning the return value, I am ok with it. It is more acceptable than an exception in the sense that the current clients of the code would not be affected. @ktf what do you think ?

@ktf
Copy link
Member

ktf commented Jan 27, 2022

I am not sure I get the why doing it client side is better. It means that the limit will be hardcoded in the C++ and therefore old releases will continue to work with a badly set / not set limit. Adjusting fine grained limits will mean a new release. If that's what we need, a fine grained rejection list on the server side seems to be the most flexible solution.
Moreover, given the object size is part of the header, the server does not need to wait for the whole payload, it can simply trust the header and reject things based on it, no?

@ktf
Copy link
Member

ktf commented Jan 27, 2022

That said, I personally do not like exceptions in core classes, but I have no strong preference for this API, if really needed.

@shahor02
Copy link
Collaborator

For the record: some calibration workflows may need to upload huge objects to in-memory transient CCDB (for the transfer to the DCS)

@Barthelemy
Copy link
Collaborator Author

@ktf the value(s) will be taken from the config files not hard-coded.

@Barthelemy
Copy link
Collaborator Author

I have added the return value and a test.

@Barthelemy
Copy link
Collaborator Author

what is going on with the mac builder ?

@costing
Copy link
Collaborator

costing commented Jan 28, 2022

@Barthelemy @ktf indeed the server can reject requests above a set threshold. It is now (tag 1.0.15, just done) configurable via the MAXPOSTSIZE environment variable (default not constrained).

For finer grained policy (i.e. per path) the request would have to be first parsed, so the transfer is going to happen anyway. Thus I agree with Barth that this should only be used as a safety net only.

@ktf
Copy link
Member

ktf commented Jan 28, 2022

You would just need to parse the headers, no? You would need to do that in any case, right? A big object is hopefully the exception not the rule. HTTP does not require you to get the payload before you return an error code, IIRC.

@ktf
Copy link
Member

ktf commented Jan 28, 2022

That said, if @Barthelemy is happy, I am happy.

@costing
Copy link
Collaborator

costing commented Jan 28, 2022

@ktf in principle yes, in practice I don't implement the full server but rely on Tomcat to do the heavy lifting. The callback to the CCDB code is after the request is handled by Tomcat.

@Barthelemy
Copy link
Collaborator Author

Why is macos failing ? can we merge ?

@ktf ktf merged commit 8005d7a into AliceO2Group:dev Jan 31, 2022
@ktf
Copy link
Member

ktf commented Jan 31, 2022

Yes, @MichaelLettrich @TimoWilken are already on the macOS issue (since last week actually).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants