HPCC-30809 Add new ESP service WsSasha #18018

wangkx · 2023-11-10T20:13:15Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

Signed-off-by: wangkx <kevin.wang@lexisnexis.com>

1. Create the sashaclilib which contains the code shared by both Sasha CLI and WsSasha. 2. Inside the sashaclilib, create the runSashaCommand() which is based on the existing code inside sasha.cpp's main() from building ISashaCommand to reading the command response. 3. Move the following from sasha.cpp to sashacli.cpp, so that they can be used by the runSashaCommand(): extractTimings, getNum(), DumpWorkunitTimings(), getVersion(). 4. Implement 5 WsSasha methods: GetVersion, ListWU, ArchiveWU, RestoreWU, and XREF. Signed-off-by: wangkx <kevin.wang@lexisnexis.com>

wangkx · 2023-11-10T23:23:44Z

@jakesmith The PR now has 2 commits for easy to review. I may squash them when needed. Please check this PR whether it is on correct direction or not. Please also comment on the questions and comments in the ecm file and other files. Some of comments may be removed before merging.

jakesmith

@wangkx - I have scanned over both commits.

1st commit looks good framework.
2nd commit, although not all of the functions, it highlights some issues.

I think for now, the scope of this should be further reduced, so we can get something past review and merged (bit size is best), then expand on it.

With that in mind, I'd keep the 1st commit, rollback the 2nd commit, and add an extremely limited service for now, e.g. that just handles SCA_GETVERSION for example.
Keep sasha.cpp more or less exactly as it was, but conditionally call runSashaCommand for the select command (e.g. SCA_GETVERSION).
In the short term, that may mean there's some duplication between the legacy sasha.cpp and saruncmd.cpp.

This PR and review can then focus on the new service and the impl. of runSashaCommand etc. - subsequent PR's can start moving more functionality to the service.

jakesmith · 2023-11-15T11:29:43Z

esp/services/ws_sasha/ws_sashaservice.cpp


 using namespace ws_sasha;

+constexpr const char* FEATURE_URL = "SashaAccess";
+
+ISashaCommand* CWSSashaEx::setSashaCommand(SashaCommandAction action, bool dfu, SocketEndpoint& serverep)


not sure why setXXX , it is a createXX method ?

I did not use a createXX method because the createSashaCommand() (defined in sacmd.hpp) is called in line 34. I may rename this one to createAndSetSashaCommand.

jakesmith · 2023-11-15T11:36:03Z

dali/sasha/sashacli.hpp

+
+#include "dautils.hpp"
+
+extern SASHACLI_API void runSashaCommand(SocketEndpoint ep, ISashaCommand *cmd, IFileIOStream *outfile, StringBuffer &outBuffer, bool viaESP);


SocketEndpoint ep

don't want a copy - should probably be const reference

jakesmith · 2023-11-15T11:43:06Z

dali/sasha/sashacli.cpp

+            else
+                msg.append("archived");
+            msg.append(", Continue (Y/N)");
+            if (!confirm(msg.str()))


this likely need to change..
Anything that it is confirming to proceed, is running 2 commands (but it's wrapped at the moment into a single runSashaCommand).

Esp and the command line will likely handle asking to proceed in very different ways.
I think this logic should be split so runSashaCommand only ever runs 1 command, and in the cases of commands that want confirmation and then optionally run another, that logic is elsewhere, e.g. in the command line tool, or eclwatch.

Possibly move out of the runSashaCommand()

Yes, and issue each command separately, i.e. split the bit after the confirm into a separate call to runSashaCommand

jakesmith · 2023-11-15T11:44:48Z

dali/sasha/sashacli.cpp

+            PROGLOG("Sasha server[%s]: Version %s",host.str(),id.str());
+        return;
+    }
+    if (viaESP)


I don't think any of this common code should have/switch on viaESP.
Instead, it should behave the same, e.g. throw an exception, the caller should handle it as it wants to, e.g. in case of CLI - log it.

jakesmith · 2023-11-15T11:46:28Z

dali/sasha/sashacli.cpp

It is no longer really "cli" .. since it's used for sasha command handling in general / by anything that wants to run command on the Sasha service.
Perhaps rename saruncmd.cpp

jakesmith · 2023-11-15T11:52:42Z

esp/scm/ws_sasha.ecm

+
+    //ESPmethod [auth_feature("SashaAccess:READ")] ListDT(ListDTRequest, ResultResponse);
+        // By reading the code, looks like: it lists ECL WUs or DFU WUs with date time (when?) in Archive server.
+        // Should it be merged into the ListWU as an option (IncludeDateTime)?


probably, and related to prev. comment, if confirmed, then it would be fine to merge into single ecm request type with options, but I would not change the handling code that is moving from sasha.cpp into the service - or change as little as possible.

jakesmith · 2023-11-15T11:54:54Z

esp/scm/ws_sasha.ecm

+    ESPmethod [auth_feature("SashaAccess:FULL")] RestoreWU(RestoreWURequest, ResultResponse); //restore ECL WUs or DFU WUs
+
+    //ESPmethod [auth_feature("SashaAccess:FULL")] BackupWU(BackupWURequest, ResultResponse); //backup one ECL WU
+    // Only backup an ECL WU (not DFU WU). How does it work? How to test? How about implementing in separate PR?


afaik, backing up/restoring dfu workunits is not supported (should be, but less of an issue because they are always tiny compared to ECL workunits).

How to test?
Not sure what you mean - you should be able to test backing up and restoring a ECL workunit manually, via the CLI, (and restore via Eclwatch)

I should ask: what is the difference between Archive and Backup?

They are the same, except Archive deletes the workunit from Dali as well, whereas Backup leave it in place.

jakesmith · 2023-11-15T11:56:29Z

esp/scm/ws_sasha.ecm

+    ESPmethod [auth_feature("SashaAccess:FULL")] XREF(XREFRequest, ResultResponse); //generate XREF for clusters
+
+    //ESPmethod [auth_feature("SashaAccess:FULL")] Stop(StopRequest, ResultResponse);
+    // Stop what? How does it work? How to test? How about implementing in separate PR?


should perhaps be a Stop or Restart request.

How to test?

Not sure what you mean using existing CLI : sasha action=stop

The Stop here matches with the existing CLI : sasha action=stop. Looks like that a Stop action (SCA_STOP) terminates a Sasha server. I haven't seen a Restart function.

There isn't a restart, I was suggesting there perhaps should be (it is prob. more useful and a more common thing to want than to stop) - but don't add now, let's keep changes to a minimum.

jakesmith · 2023-11-15T12:00:27Z

esp/scm/ws_sasha.ecm

+    // Stop what? How does it work? How to test? How about implementing in separate PR?
+
+    //Do we need the following methods? They are not in the Sasha Usage, but, in Sasha code. If yes, how about implementing in separate PRs?
+        //SCA_WORKUNIT_SERVICES_GET ?


I don't think needed by CLI or service, but still needed, i.e. components (like esp) that interact with Sasha directly, should still continue to allow this command to be created and sent to Sasha directly.

What does the SCA_WORKUNIT_SERVICES_GET (as well as coalesce_suspend and coalesce_resume) do?

SCA_WORKUNIT_SERVICES_GET

I can't recall exactly, but I don't think this new service needs to support it for now, it will not be something wanted by the CLI or UI, but instead used by other esp services that talk directly to sasha (afaics only used by wsWorkunitList atm).

coalesce_suspend and coalesce_resume)

One of sasha's function is to coalesce the dali store (aggregate the current store + in flight deltas into a new store file).
It is resource intensive, and I think the idea was to pause it, so other resource intensive sasha services like XREF, could have more capacity.
In cloud, I don't think coalesce_suspend/coalesce_resume make much sense, since each sasha service is in it's own dedicated container.

jakesmith · 2023-11-15T12:00:37Z

esp/scm/ws_sasha.ecm

+    //Do we need the following methods? They are not in the Sasha Usage, but, in Sasha code. If yes, how about implementing in separate PRs?
+        //SCA_WORKUNIT_SERVICES_GET ?
+        //coalesce_suspend ?
+        //coalesce_resume ?


should be added, but later.

wangkx · 2024-01-24T20:09:43Z

This draft PR is not needed.

HPCC-30408 Add ws_sasha framework

c2ac378

Signed-off-by: wangkx <kevin.wang@lexisnexis.com>

wangkx closed this Nov 10, 2023

wangkx reopened this Nov 10, 2023

wangkx force-pushed the h30809 branch from e5539ba to d85a8bc Compare November 10, 2023 22:18

wangkx requested a review from jakesmith November 10, 2023 23:23

wangkx changed the title ~~H30809 Add new ESP service WsSasha~~ HPCC-30809 Add new ESP service WsSasha Nov 13, 2023

jakesmith reviewed Nov 15, 2023

View reviewed changes

wangkx mentioned this pull request Nov 21, 2023

HPCC-30809 Add ESP WsSasha service and implement 3 methods #18065

Merged

39 tasks

wangkx closed this Jan 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30809 Add new ESP service WsSasha #18018

HPCC-30809 Add new ESP service WsSasha #18018

wangkx commented Nov 10, 2023

wangkx commented Nov 10, 2023

jakesmith left a comment

jakesmith Nov 15, 2023

wangkx Nov 17, 2023

jakesmith Nov 15, 2023

jakesmith Nov 15, 2023

wangkx Nov 15, 2023

jakesmith Nov 16, 2023

jakesmith Nov 15, 2023

jakesmith Nov 15, 2023

jakesmith Nov 15, 2023

jakesmith Nov 15, 2023

wangkx Nov 15, 2023

jakesmith Nov 16, 2023

jakesmith Nov 15, 2023

wangkx Nov 15, 2023

jakesmith Nov 16, 2023

jakesmith Nov 15, 2023

wangkx Nov 15, 2023

jakesmith Nov 16, 2023

jakesmith Nov 16, 2023

jakesmith Nov 15, 2023

wangkx commented Jan 24, 2024


		#include "dautils.hpp"

		extern SASHACLI_API void runSashaCommand(SocketEndpoint ep, ISashaCommand cmd, IFileIOStream outfile, StringBuffer &outBuffer, bool viaESP);

HPCC-30809 Add new ESP service WsSasha #18018

HPCC-30809 Add new ESP service WsSasha #18018

Conversation

wangkx commented Nov 10, 2023

Type of change:

Checklist:

Smoketest:

Testing:

wangkx commented Nov 10, 2023

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkx commented Jan 24, 2024