slicer: Do not put link objects that will be rejected #566
Conversation
carpawell
requested review from
roman-khimov,
cthulhu-rider and
smallhive
as code owners
|
However, we also may allow LINK objects to be bigger than regular object size limit. Also, that is not really helpful if the whole object is PUT already and only the last iteration has failed. A user cannot even know any ID. |
| @@ -581,6 +581,10 @@ func (x *PayloadWriter) _writeChild(ctx context.Context, meta dynamicObjectMetad | |||
| payload := obj.Payload() | |||
| payloadAsBuffers := [][]byte{obj.Payload()} | |||
|
|
|||
| if uint64(len(payload)) > x.payloadSizeLimit { | |||
There was a problem hiding this comment.
@carpawell did u think about what will happen if we'll make recursion here? it will definitely converge (if x.payloadSizeLimit > 32+eps tbh), but whether the system is ready for this is a question
the 2nd question is what payload limit will be with x.payloadSizeLimit=64M?
There was a problem hiding this comment.
Link objects are flat simple ones only for now. Extension can be made in the future, but I doubt it's needed.
There was a problem hiding this comment.
ok, i was just wondering. Metadata tracking could need extensions for this, placement/assembly would work i guess
There was a problem hiding this comment.
@carpawell did u think about what will happen if we'll make recursion here?
that will require node-side adoption for sure, and it is hard to predict how much of a change it will take
the 2nd question is what payload limit will be with x.payloadSizeLimit=64M?
1560671 parts can be attached to the payload (43 bytes is for 64mb varint + OID), that is 95TB
| @@ -581,6 +581,10 @@ func (x *PayloadWriter) _writeChild(ctx context.Context, meta dynamicObjectMetad | |||
| payload := obj.Payload() | |||
| payloadAsBuffers := [][]byte{obj.Payload()} | |||
|
|
|||
| if uint64(len(payload)) > x.payloadSizeLimit { | |||
There was a problem hiding this comment.
Link objects are flat simple ones only for now. Extension can be made in the future, but I doubt it's needed.
| @@ -581,6 +581,10 @@ func (x *PayloadWriter) _writeChild(ctx context.Context, meta dynamicObjectMetad | |||
| payload := obj.Payload() | |||
| payloadAsBuffers := [][]byte{obj.Payload()} | |||
|
|
|||
| if uint64(len(payload)) > x.payloadSizeLimit { | |||
| return fmt.Errorf("link's payload exceeds max available size: %d > %d", uint64(len(payload)), x.payloadSizeLimit) | |||
There was a problem hiding this comment.
at this point we have complete chain accepted except linker, right? So, this makes the original object alive in the system, and we've done a lot of work. But here we fail. From one side, this looks pretty legit - we cannot form the complete chain. From the other one, we've made "enough" chain. What im thinking bout are:
- will system auto-remove the chain eventually? this could be a huge pile of data user doesnt expect to reside in his container
- if not, may be we'll let such chains to live and respond to user about this?
the case could be generalized to any problems with linker storage, but slicing is even more interesting
good to be discussed in a separate issue, but mb u already thought
There was a problem hiding this comment.
We still have a valid object. Maybe it's better be controlled by some maximum object size limit (which can fail earlier than this one, but it'll leave incomplete chain then).
There was a problem hiding this comment.
there will always be (potential) situations when a user did not know the exact size and we have to stop splitting when some objects are already PUT, so i see no good solution except accepting them and marking garbage only if an additional user command is received (e.g. vacuum) OR sending some signed request from the user/slicer that the whole split was a mistake and parts should be marked for deletion
|
@carpawell tests with small limit broke, should be fixed. For example, we can predict whether links will fit into the object, and if not, expect an introduced error. Tested limits' increase could also be needed |
Since link objects have payload now, they may overflow max regular object size. Client-size error is better that server-side overflow message. Not expected in practice but may be observed when running some extreme local installations for tests (e.g., 1K limit and 100K object). Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell
force-pushed
the
feat/more-valuable-error-on-link-overflow
branch
from
7ffed53 to
f5ffd70
Compare
Just increased the limits, if you don't mind. There is not much we can do here.
Only if we know the exact object size. That is not granted. And i think in practice a known object size is not the case where it cannot fit a link. Not sure what can store 95TB, know its size and stream it to the NeoFS. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #566 +/- ##
=========================================
Coverage ? 68.15%
=========================================
Files ? 122
Lines ? 9964
Branches ? 0
=========================================
Hits ? 6791
Misses ? 2799
Partials ? 374 ☔ View full report in Codecov by Sentry. |
Since link objects have payload now, they may overflow max regular object size. Client-size error is better that server-side overflow message. Not expected in practice but may be observed when running some extreme local installations for tests (e.g., 1K limit and 100K object).