Skip to content

Commit

Permalink
issue-1350: replacing TABLET_VERIFY with impossible events + errors i…
Browse files Browse the repository at this point in the history 
…n CreateNode and CreateHandle (#1558)
  • Loading branch information
@qkrorlqr
qkrorlqr authored Jul 7, 2024
1 parent 1601f56 commit fac35ab
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 28 deletions.
7 changes: 7 additions & 0 deletions cloud/filestore/libs/diagnostics/critical_events.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@ namespace NCloud::NFileStore{

#define FILESTORE_IMPOSSIBLE_EVENTS(xxx) \
xxx(CancelRoutineIsNotSet) \
xxx(ChildNodeWithoutRef) \
xxx(SessionNotFoundInTx) \
xxx(InvalidNodeIdForLocalNode) \
xxx(ChildNodeIsNull) \
xxx(TargetNodeWithoutRef) \
xxx(ParentNodeIsNull) \
xxx(FailedToCreateHandle) \
// FILESTORE_IMPOSSIBLE_EVENTS

////////////////////////////////////////////////////////////////////////////////
Expand Down
43 changes: 35 additions & 8 deletions cloud/filestore/libs/storage/tablet/tablet_actor_createhandle.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "helpers.h"

#include <cloud/filestore/libs/diagnostics/critical_events.h>
#include <cloud/filestore/libs/storage/api/tablet_proxy.h>

#include <util/generic/guid.h>
Expand Down Expand Up @@ -271,7 +272,7 @@ void TIndexTabletActor::HandleCreateHandle(
ExecuteTx<TCreateHandle>(
ctx,
std::move(requestInfo),
msg->Record);
std::move(msg->Record));
}

////////////////////////////////////////////////////////////////////////////////
Expand All @@ -289,7 +290,12 @@ bool TIndexTabletActor::PrepareTx_CreateHandle(
args.ClientId,
args.SessionId,
args.SessionSeqNo);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return true;
}

args.ReadCommitId = GetReadCommitId(session->GetCheckpointId());
if (args.ReadCommitId == InvalidCommitId) {
Expand Down Expand Up @@ -377,7 +383,6 @@ bool TIndexTabletActor::PrepareTx_CreateHandle(
}

// TODO: AccessCheck
TABLET_VERIFY(args.TargetNode);
}

return true;
Expand All @@ -391,7 +396,12 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
FILESTORE_VALIDATE_TX_ERROR(CreateHandle, args);

auto* session = FindSession(args.SessionId);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

// TODO: check if session is read only
args.WriteCommitId = GenerateCommitId();
Expand All @@ -404,8 +414,19 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
if (args.TargetNodeId == InvalidNodeId
&& (args.FollowerId.Empty() || args.IsNewFollowerNode))
{
TABLET_VERIFY(!args.TargetNode);
TABLET_VERIFY(args.ParentNode);
if (args.TargetNode) {
auto message = ReportTargetNodeWithoutRef(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

if (!args.ParentNode) {
auto message = ReportParentNodeIsNull(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

if (args.FollowerId.Empty()) {
NProto::TNode attrs =
Expand Down Expand Up @@ -473,7 +494,13 @@ void TIndexTabletActor::ExecuteTx_CreateHandle(
session->GetCheckpointId() ? args.ReadCommitId : InvalidCommitId,
args.Flags);

TABLET_VERIFY(handle);
if (!handle) {
auto message = ReportFailedToCreateHandle(TStringBuilder()
<< "CreateHandle: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

args.Response.SetHandle(handle->GetHandle());
auto* node = args.Response.MutableNodeAttr();
ConvertNodeFromAttrs(*node, args.TargetNodeId, args.TargetNode->Attrs);
Expand Down Expand Up @@ -526,7 +553,7 @@ void TIndexTabletActor::CompleteTx_CreateHandle(
args.FollowerName,
ctx.SelfID,
CreateRegularAttrs(args.Mode, args.Uid, args.Gid),
std::move(args.Headers),
std::move(*args.Request.MutableHeaders()),
std::move(args.Response));

auto actorId = NCloud::Register(ctx, std::move(actor));
Expand Down
40 changes: 31 additions & 9 deletions cloud/filestore/libs/storage/tablet/tablet_actor_createnode.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "helpers.h"

#include <cloud/filestore/libs/diagnostics/critical_events.h>
#include <cloud/filestore/libs/storage/api/tablet_proxy.h>

namespace NCloud::NFileStore::NStorage {
Expand Down Expand Up @@ -269,10 +270,10 @@ void TIndexTabletActor::HandleCreateNode(
ExecuteTx<TCreateNode>(
ctx,
std::move(requestInfo),
msg->Record,
std::move(msg->Record),
parentNodeId,
targetNodeId,
attrs);
std::move(attrs));
}

////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -312,7 +313,6 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
}

// TODO: AccessCheck
TABLET_VERIFY(args.ParentNode);

// validate target node doesn't exist
TMaybe<IIndexTabletDatabase::TNodeRef> childRef;
Expand All @@ -326,7 +326,13 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
return true;
}

TABLET_VERIFY(!args.ChildNode);
if (args.ChildNode) {
auto message = ReportChildNodeWithoutRef(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return true;
}

if (args.TargetNodeId != InvalidNodeId) {
// hard link: validate link target
args.ChildNodeId = args.TargetNodeId;
Expand All @@ -353,7 +359,6 @@ bool TIndexTabletActor::PrepareTx_CreateNode(
}

// TODO: AccessCheck
TABLET_VERIFY(args.ChildNode);
}

return true;
Expand All @@ -367,7 +372,12 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
FILESTORE_VALIDATE_TX_ERROR(CreateNode, args);

auto* session = FindSession(args.SessionId);
TABLET_VERIFY(session);
if (!session) {
auto message = ReportSessionNotFoundInTx(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

TIndexTabletDatabase db(tx.DB);

Expand Down Expand Up @@ -424,7 +434,12 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
args.FollowerName);

if (args.FollowerId.Empty()) {
TABLET_VERIFY(args.ChildNodeId != InvalidNodeId);
if (args.ChildNodeId == InvalidNodeId) {
auto message = ReportInvalidNodeIdForLocalNode(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
args.Error = MakeError(E_INVALID_STATE, std::move(message));
return;
}

ConvertNodeFromAttrs(
*args.Response.MutableNode(),
Expand All @@ -445,7 +460,9 @@ void TIndexTabletActor::ExecuteTx_CreateNode(
}
}

// TODO(#1350): support DupCache for external nodes
// TODO(#1350): support DupCache for external nodes - modify DupCache entry
// upon CreateNode completion in follower - in the same tx that would
// delete the corresponding CreateNode op from the op log table
}

void TIndexTabletActor::CompleteTx_CreateNode(
Expand Down Expand Up @@ -487,7 +504,12 @@ void TIndexTabletActor::CompleteTx_CreateNode(
CommitDupCacheEntry(args.SessionId, args.RequestId);
}

TABLET_VERIFY(args.ChildNode);
if (!args.ChildNode) {
auto message = ReportChildNodeIsNull(TStringBuilder()
<< "CreateNode: " << args.Request.ShortDebugString());
*args.Response.MutableError() =
MakeError(E_INVALID_STATE, std::move(message));
}
response->Record = std::move(args.Response);

NProto::TSessionEvent sessionEvent;
Expand Down
18 changes: 7 additions & 11 deletions cloud/filestore/libs/storage/tablet/tablet_tx.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,22 +616,20 @@ struct TTxIndexTablet

TCreateNode(
TRequestInfoPtr requestInfo,
const NProto::TCreateNodeRequest request,
NProto::TCreateNodeRequest request,
ui64 parentNodeId,
ui64 targetNodeId,
const NProto::TNode& attrs)
NProto::TNode attrs)
: TSessionAware(request)
, RequestInfo(std::move(requestInfo))
, ParentNodeId(parentNodeId)
, TargetNodeId(targetNodeId)
, Name(request.GetName())
, Attrs(attrs)
, Attrs(std::move(attrs))
, FollowerId(request.GetFollowerFileSystemId())
, FollowerName(CreateGuidAsString())
, Request(std::move(request))
{
if (FollowerId) {
Request = request;
}
}

void Clear()
Expand Down Expand Up @@ -1061,7 +1059,7 @@ struct TTxIndexTablet
const ui32 Uid;
const ui32 Gid;
const TString RequestFollowerId;
NProto::THeaders Headers;
NProto::TCreateHandleRequest Request;

ui64 ReadCommitId = InvalidCommitId;
ui64 WriteCommitId = InvalidCommitId;
Expand All @@ -1076,7 +1074,7 @@ struct TTxIndexTablet

TCreateHandle(
TRequestInfoPtr requestInfo,
const NProto::TCreateHandleRequest& request)
NProto::TCreateHandleRequest request)
: TSessionAware(request)
, RequestInfo(std::move(requestInfo))
, NodeId(request.GetNodeId())
Expand All @@ -1086,10 +1084,8 @@ struct TTxIndexTablet
, Uid(request.GetUid())
, Gid(request.GetGid())
, RequestFollowerId(request.GetFollowerFileSystemId())
, Request(std::move(request))
{
if (RequestFollowerId) {
Headers = request.GetHeaders();
}
}

void Clear()
Expand Down

0 comments on commit fac35ab

Please sign in to comment.