Skip to content

Commit

Permalink
Code review changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@jpmcmu
jpmcmu committed Jan 8, 2025
1 parent 6aa073f commit 3b0458d
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 98 deletions.
34 changes: 14 additions & 20 deletions fs/dafilesrv/dafilesrv.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,24 +366,6 @@ version: 1.0
detail: 100
)!!";

IPropertyTree * loadConfigurationWithGlobalDefault(const char * defaultYaml, Owned<IPropertyTree>& globalConfig, const char * * argv, const char * componentTag, const char * envPrefix)
{
Owned<IPropertyTree> componentDefault;
if (defaultYaml)
{
Owned<IPropertyTree> defaultConfig = createPTreeFromYAMLString(defaultYaml, 0, ptr_ignoreWhiteSpace, nullptr);
componentDefault.set(defaultConfig->queryPropTree(componentTag));
if (!componentDefault)
throw makeStringExceptionV(99, "Default configuration does not contain the tag %s", componentTag);
}
else
componentDefault.setown(createPTree(componentTag));

mergePTree(componentDefault, globalConfig);

return loadConfiguration(componentDefault, argv, componentTag, envPrefix, nullptr, nullptr);
}

int main(int argc, const char* argv[])
{
InitModuleObjects();
Expand All @@ -408,13 +390,25 @@ int main(int argc, const char* argv[])

#ifndef _CONTAINERIZED
Owned<IPropertyTree> env = getHPCCEnvironment();
IPropertyTree* globalTracing = env->queryPropTree("Software/tracing");
IPropertyTree* globalTracing = env->getPropTree("Software/tracing");
if (globalTracing != nullptr)
extractedGlobalConfig->addPropTree("tracing", globalTracing);
#endif

const char* componentTag = "dafilesrv";
Owned<IPropertyTree> componentDefault;
if (defaultYaml)
{
Owned<IPropertyTree> defaultConfig = createPTreeFromYAMLString(defaultYaml, 0, ptr_ignoreWhiteSpace, nullptr);
componentDefault.set(defaultConfig->queryPropTree(componentTag));
if (!componentDefault)
throw makeStringExceptionV(99, "Default configuration does not contain the tag %s", componentTag);
}
else
componentDefault.setown(createPTree(componentTag));

// NB: bare-metal dafilesrv does not have a component specific xml, extracting relevant global configuration instead
Owned<IPropertyTree> config = loadConfigurationWithGlobalDefault(defaultYaml, extractedGlobalConfig, argv, "dafilesrv", "DAFILESRV");
Owned<IPropertyTree> config = loadConfiguration(componentDefault, extractedGlobalConfig, argv, componentTag, "DAFILESRV", nullptr, nullptr);

Owned<IPropertyTree> keyPairInfo; // NB: not used in containerized mode
// Get SSL Settings
Expand Down
90 changes: 12 additions & 78 deletions fs/dafsserver/dafsserver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,69 +162,6 @@ static ISecureSocket *createSecureSocket(ISocket *sock, bool disableClientCertVe
}
#endif

//------------------------------------------------------------------------------
// ActiveSpanScope Design Notes:
//------------------------------------------------------------------------------
// ActiveSpanScope updates the threadActiveSpan when it is intstantiated
// and restores it to a user configurable previous ISpan when it leaves scope.
// ActiveSpanScope does not control its referenced ISpan's lifetime or ending.
//
// This design allows ActiveSpanScope to be used to update threadActiveSpan
// for long running ISpans that are time sliced, worked on from multiple threads,
// and / or passed between threads. In these cases multiple ActiveSpanScopes
// will be created over the lifetime of the referenced of the ISpan to represent
// a slice of work done towards that ISpan.
//
// Allowing the previous / restored ISpan to be configured allows for
// "disconnected" work on ISpans to be done. Where the previously active ISpan
// may not be the correct ISpan to restore when an ActiveSpanScope leaves scope.
//
// When an ActiveSpanScope is destroyed it will return the prevSpan to active,
// if and only if its span is still the threadActiveSpan. If this is not this
// implies that there is a conflicting ActiveSpanScope and that a code structure
// issue exists that needs to be addressed. A IERRLOG message will be added
// in debug builds for these cases.
//------------------------------------------------------------------------------

class ActiveSpanScope
{
public:
// Captures current threadActiveSpan for prevSpan
ActiveSpanScope(ISpan * _ptr) : ActiveSpanScope(_ptr, queryThreadedActiveSpan()) {}
ActiveSpanScope(ISpan * _ptr, ISpan * _prev) : span(_ptr), prevSpan(_prev)
{
setThreadedActiveSpan(_ptr);
}
ActiveSpanScope(const ActiveSpanScope& rhs) = delete;

~ActiveSpanScope()
{
ISpan* current = queryThreadedActiveSpan();
if (current != span)
{
const char* currSpanID = current != nullptr ? current->querySpanId() : "null";
const char* expectedSpanID = span != nullptr ? span->querySpanId() : "null";

IERRLOG("~ActiveSpanScope: threadActiveSpan has changed unexpectedly, expected: %s actual: %s", expectedSpanID, currSpanID);
return;
}

setThreadedActiveSpan(prevSpan);
}

inline ISpan * operator -> () const { return span; }
inline operator ISpan *() const { return span; }

inline ActiveSpanScope& operator=(ISpan * ptr) = delete;
inline ActiveSpanScope& operator=(const ActiveSpanScope& rhs) = delete;

inline bool operator == (ISpan * _ptr) const { return span == _ptr; }
inline bool operator != (ISpan * _ptr) const { return span != _ptr; }
private:
ISpan * span = nullptr;
ISpan * prevSpan = nullptr;
};

static void reportFailedSecureAccepts(const char *context, IException *exception, unsigned &numFailedConn, unsigned &timeLastLog)
{
numFailedConn++;
Expand Down Expand Up @@ -902,7 +839,7 @@ class CRemoteRequest : public CSimpleInterfaceOf<IInterface>
MemoryBuffer expandMb;
Owned<IXmlWriterExt> responseWriter; // for xml or json response

Owned<ISpan> requestSpan;
OwnedSpanLifetime requestSpan;
std::string requestTraceParent;

bool handleFull(MemoryBuffer &inMb, size32_t inPos, MemoryBuffer &compressMb, ICompressor *compressor, size32_t replyLimit, size32_t &totalSz)
Expand Down Expand Up @@ -1164,7 +1101,6 @@ class CRemoteRequest : public CSimpleInterfaceOf<IInterface>
if (requestSpan != nullptr)
{
requestSpan->setSpanStatusSuccess(true);
requestSpan->endSpan();
}
}

Expand All @@ -1178,33 +1114,31 @@ class CRemoteRequest : public CSimpleInterfaceOf<IInterface>
const char* fullTraceContext = requestTree->queryProp("_trace/traceparent");

// We only want to compare the trace-id & span-id, so remove the last "sampling" group
std::string traceParent = fullTraceContext ? fullTraceContext : "";
traceParent = traceParent.substr(0,traceParent.find_last_of("-"));
const char* traceParent = fullTraceContext ? fullTraceContext : "";
traceParent = strrchr(traceParent, '-');

if (!traceParent.empty() && requestTraceParent != traceParent)
if (strlen(traceParent) != 0 && requestTraceParent != traceParent)
{
// Check to see if we have an existing span that needs to be closed out, this can happen
// when the span parent changes on the client side
// Check to see if we have an existing span that needs to be marked successful before close
if (requestSpan != nullptr)
{
requestSpan->setSpanStatusSuccess(true);
requestSpan->endSpan();
}

Owned<IProperties> traceHeaders = createProperties();
traceHeaders->setProp("traceparent", fullTraceContext);

std::string requestSpanName;
const char* requestSpanName = nullptr;
if (activity->queryIsReadActivity())
requestSpanName = "ReadRequest";
else
requestSpanName = "WriteRequest";

requestSpan.set(queryTraceManager().createServerSpan(requestSpanName.c_str(), traceHeaders));
requestSpan.set(queryTraceManager().createServerSpan(requestSpanName, traceHeaders));
requestTraceParent = traceParent;
}

ActiveSpanScope activeSpan(requestSpan.get());
ActiveSpanScope activeSpan(requestSpan.query());

if (requestTree->hasProp("replyLimit"))
replyLimit = requestTree->getPropInt64("replyLimit", defaultDaFSReplyLimitKB) * 1024;
Expand Down Expand Up @@ -5067,7 +5001,7 @@ class CRemoteFileServer : implements IRemoteFileServer, public CInterface
}
case StreamCmd::CLOSE:
{
OwnedSpanScope closeSpan;
OwnedActiveSpanScope closeSpan;
const char* traceParent = requestTree->queryProp("_trace/traceparent");
if (traceParent != nullptr)
{
Expand All @@ -5079,7 +5013,7 @@ class CRemoteFileServer : implements IRemoteFileServer, public CInterface

if (0 == cursorHandle)
{
IException* exception = createDafsException(DAFSERR_cmdstream_protocol_failure, "cursor handle not supplied to 'close' command");
IDAFS_Exception* exception = createDafsException(DAFSERR_cmdstream_protocol_failure, "cursor handle not supplied to 'close' command");
closeSpan->recordException(exception);
throw exception;
}
Expand Down Expand Up @@ -5112,14 +5046,14 @@ class CRemoteFileServer : implements IRemoteFileServer, public CInterface
{
case StreamCmd::VERSION:
{
OwnedSpanScope versionSpan;
OwnedActiveSpanScope versionSpan;
const char* traceParent = requestTree->queryProp("_trace/traceparent");
if (traceParent != nullptr)
{
Owned<IProperties> traceHeaders = createProperties();
traceHeaders->setProp("traceparent", traceParent);

versionSpan = queryTraceManager().createServerSpan("VersionRequest", traceHeaders);
versionSpan.set(queryTraceManager().createServerSpan("VersionRequest", traceHeaders));
}

if (outFmt_Binary == outputFormat)
Expand Down

0 comments on commit 3b0458d

Please sign in to comment.