when the client tries to push two consecutive chunks below the database limit

Can you clarify what you mean by this? I'm a bit confused

Yes, I was afraid this wouldn't be super clear right away.

So, we have a DB limit which is 400kb. If we upload data that exceeds this limit, it is stored on the S3, otherwise, we save it in the DB.

Now, what happens when we push a single chunk < 400kb is just that it is saved in the DB and that works ok.

But the scenario that is covered here is when a client pushes two (or more) chunks < DB limit, let's say 2 chunks of 100kb each for a single log.
Without this change, what happens is that we receive the first chunk, store it in the DB and return the OK status. Other chunks may be pushed but are never really used so they are lost and still the client sees the OK status as a result of this operation.
With this change, we continue the work of the reactor and listen for more chunks. If the client pushes more chunks after the first one is under the DB limit, we return an error. This is correct behavior as pushing multiple small chunks produces redundant grpc cycles and it means that the client code is poorly designed. Basically, in this case, the chunks should be merged on the client-side.

Ah okay that explanation makes sense! Now I'm just a bit confused about how this change achieves what you described...

1. Does return nullptr cause the error to be propagated?
2. How do we make sure we cause the error to be propagated only on the second chunk, rather than on the first chunk?

1. Does return nullptr cause the error to be propagated?

It causes the error to be triggered when a situation like I described above occurs. Previously, it would never be triggered.

2. How do we make sure we cause the error to be propagated only on the second chunk, rather than on the first chunk?

We perform checks on the persistenceMethod (by small chunk I'm going to mean the chunk below the DB limit, the big one - above it):

• if you push first a small chunk and then a big one - there will be an error because we first set the persistenceMethod to DB and then try to push further.
• if you push small and small - the error will be triggered - that's the case we're handling in this diff
• if you push a big and small/big - it'll be successfully stored on the S3

Basically, if you push a small chunk, you should stop right there, because we don't allow any extra further data. We could handle a situation when you push a couple of really small chunks that would sum into a small chunk (< 400kb) eventually, but I do not see a point in doing so, we can just push 400kb over grpc and avoid making it more complicated than it has to be.

Thanks, that mostly makes sense, but one more question:

In D4232#119697, @karol-bisztyga wrote:

1. Does return nullptr cause the error to be propagated?

It causes the error to be triggered when a situation like I described above occurs. Previously, it would never be triggered.

Are you saying return nullptr will cause the error to be propagated? Or are you saying that return nullptr will put the Reactor in such a state that if it were to receive another chunk, it would then crash?

If it's the latter, it would seem like return nullptr is basically saying "we are done, and don't expect to receive anything else". But normally I would expect std::make_unique<grpc::Status>(grpc::Status::OK) (the old code) to be saying "we are done".

Can you clarify what return nullptr does and what std::make_unique<grpc::Status>(grpc::Status::OK) does?