Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-18408 Create an interface to hide dali esdl access details #10653

Merged
merged 1 commit into from Feb 27, 2018

Conversation

Projects
None yet
7 participants
@mayx
Copy link
Member

commented Nov 16, 2017

  • Add interface IEsdlStore to abstract esdl dali operations
  • Move dali related operations from esdl binding and ws_esdlconfig to
    a class that implements IEsdlStore. Most of the implementation function
    remain unchanged during the move.
  • Add IEsdlSubscription and IEsdlListener interfaces to abstract
    subscription and notify operations with dali

Signed-off-by: mayx yanrui.ma@lexisnexisrisk.com

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:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Testing:

@hpcc-jirabot

This comment has been minimized.

Copy link

commented Nov 16, 2017

@mayx

This comment has been minimized.

Copy link
Member Author

commented Nov 16, 2017

@afishbeck @rpastrana Please code review...

@mayx mayx force-pushed the mayx:HPCC-18408-new branch from fe0a13f to 354a55d Nov 16, 2017

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

@mayx can you check into why smoketest failed

@rpastrana
Copy link
Member

left a comment

@mayx a couple of minor comments, and a couple of architectural concerns regarding the dali interaction improvements suggested by @jakesmith. We can meet and discuss offline.

@@ -0,0 +1,82 @@
/*##############################################################################
HPCC SYSTEMS software Copyright (C) 2013 HPCC Systems®.

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

update the year on new files

@@ -413,10 +149,10 @@ IPropertyTree * CWsESDLConfigEx::getEspProcessRegistry(const char * espprocname,
Owned<IRemoteConnection> globalLock = querySDS().connect(xpath.str(), myProcessSession(), RTM_LOCK_READ, SDS_LOCK_TIMEOUT_DESDL);

if (!globalLock)
throw MakeStringException(-1, "Unable to connect to ESDL Service configuration information in dali %s", ESDL_DEFS_ROOT_PATH);
throw MakeStringException(-1, "Unable to connect to esp configuration information in dali %s", xpath.str());

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

Capitalize ESP

if (oldEnvironment.get())
StringBuffer errmsg, thexml;
bool deleted = m_esdlStore->deleteDefinition(esdlDefinitionId.str(), errmsg, thexml);
if (deleted)

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

very minor, but unless we need deleted afterward, then just test
if ( m_esdlStore->deleteDefinition(esdlDefinitionId.str(), errmsg, thexml))

@@ -0,0 +1,865 @@
/*##############################################################################
HPCC SYSTEMS software Copyright (C) 2013 HPCC Systems®.

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

update date on new files.

#include <memory>

static const char* ESDL_DEFS_ROOT_PATH="/ESDL/Definitions/";
static const char* ESDL_DEF_PATH="/ESDL/Definitions/Definition";

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

It's my understanding that the major performance gain would come from changing the Definitions to be uniquely identifiable based on the element name...
ie
/ESDL/Definitions/Definition @id=mathservice.1 would turn to
/ESDL/Definitions/mathservice.1

two points...
Although that is the one performance improvement suggested by @jakesmith, I'm not sure if the performance improvement potential is high enough to pursue or not, but I do believe that's what Jake pointed out.
Also, I'm not sure if now we need to set a nomenclature rule for the definition names. in other words, I don't know if allowing the Definitions to be named freely by the developer is ok or not, or if we need to come up w/ a standard naming convention like the workunits... /Workunits/W

This comment has been minimized.

Copy link
@mayx

mayx Nov 17, 2017

Author Member

@rpastrana the scope of this issue is to create the interfaces and encapsulate the Dali operations. There's another issue for the performance improvement(HPCC-18492). The 2 concerns you pointed out are very good points, I'll copy over the comments to the other issue, and we'll discuss further.

}
};

class CEsdlSDSSubscription : implements IEsdlSubscription, public CInterface, implements ISDSSubscription

This comment has been minimized.

Copy link
@rpastrana

rpastrana Nov 16, 2017

Member

The other improvement @jakesmith mentioned was to minimize/eliminate the dali subscriptions all together. Instead of relying on the dali notifications, wsesdlconfig could communicate the esdl def/binding change directly to the appropriate DESDL binding/service.

This comment has been minimized.

Copy link
@mayx

mayx Nov 17, 2017

Author Member

@rpastrana again this issue is simply create the interface and set up the stage for other improvements. I've created another task for the subscription improvement: https://track.hpccsystems.com/browse/HPCC-18754, as a subtask of 18153.

This comment has been minimized.

Copy link
@jakesmith

jakesmith Nov 17, 2017

Member

@mayx - it may be beyond the scope of this PR, but from a brief look at it, you're new interfaces are tied to a subscription / notify paradigm.
Is the intention to then remove these at later point, when the updates are pushed without a subscription?

This comment has been minimized.

Copy link
@mayx

mayx Nov 27, 2017

Author Member

@jakesmith The new subscription interface is to isolate the components that will act on the changes(consumer), and the generic component that will receive the changes(producer). The producer currently is the Dali subscription mechanism, but it can also be a service that receives pushes. The Subscription in the interface is simply a bridge between the consumer and the producer for them to communicate asynchronously. In the pushing scenario, creating a subscription is simply adding the consumer to the list of consumers of the push receiver (instead of subscribing to Dali). Note: Some of the function parameters (such as espprocess and binding) may change a bit later with other planned changes.

@mayx mayx force-pushed the mayx:HPCC-18408-new branch from 354a55d to 557613b Nov 17, 2017

@mayx

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2017

@rpastrana @afishbeck changes made based on comments, please review again...

@jakesmith

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

@mayx - no other comments.
@afishbeck - may be good if you take a final review.

@afishbeck
Copy link
Member

left a comment

Comments mostly on interface level for now.

interface IEsdlStore : public IInterface
{
virtual void fetchDefinition(const char* id, StringBuffer& esxdl) = 0;
virtual void fetchDefinition(const char* esdlServiceName, unsigned targetVersion, StringBuffer& esxdl) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

In future Definitions may contain more than one service... it's potentially much more efficient because of shared structures, etc. Perhaps lets just say "definitionName", instead of serviceName?

The word target is overused... in this case rather than "targetVersion" maybe "instance" or "version" might be better.

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

It also might be less clutter just to always let the caller build the id, rather than having 2 methods, but I could be convinced either way.

This comment has been minimized.

Copy link
@mayx

mayx Jan 3, 2018

Author Member

The second fetch function removed.

#define esdl_engine_decl
#endif

interface IEsdlStore : public IInterface

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

The following comments on the interface are mostly to help make it self documenting and to sync with my long term ideas... rather than current implementation.

{
virtual void fetchDefinition(const char* id, StringBuffer& esxdl) = 0;
virtual void fetchDefinition(const char* esdlServiceName, unsigned targetVersion, StringBuffer& esxdl) = 0;
virtual void fetchDefinitionByNameOnly(const char * name, StringBuffer & def) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

fetchLatestDefinition? NameOnly is a bit confusing as the method is really treating the base name as an alias for getting the latest version of the definition.

This comment has been minimized.

Copy link
@mayx

mayx Jan 3, 2018

Author Member

Function name changed to fetchLatestDefinition.


esdl_engine_decl IEsdlStore* createEsdlCentralStore()
{
return new CEsdlSDSStore;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

This class seems fairly stateless, at least I don't think I saw member variables... can we maintain one global copy created when createEsdlCentralStore is called the first time? If nothing else it would save multiple calls to ensureSDSPath() and DALI communication.

This comment has been minimized.

Copy link
@mayx

mayx Jan 3, 2018

Author Member

Makes sense, will make the change.

virtual void fetchDefinition(const char* id, StringBuffer& esxdl) = 0;
virtual void fetchDefinition(const char* esdlServiceName, unsigned targetVersion, StringBuffer& esxdl) = 0;
virtual void fetchDefinitionByNameOnly(const char * name, StringBuffer & def) = 0;
virtual IPropertyTree* fetchBinding(const char* espProcess, const char* bindingName) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

This whole binding term gets pretty confusing because we have ESP/ENV_Configured/static bindings, and ESDL/DALI_ESDL_Configured/Virtual bindings. Wish I'd called the ESDL one something else. Anyway perhaps for this method.. fetchESDLBinding(const char *espProcess, const char *espStaticBinding); Just to be ultra clear.

Some of your other issues may change this even further because we'll really tie ESDLBindings to a port, rather than a preconfigured ESPStaticBinding. Eventually maybe we'll get the naming down so it's not so confusing.

This comment has been minimized.

Copy link
@mayx

mayx Jan 3, 2018

Author Member

Changed argument name to espStaticBinding

virtual bool existsMethodDef(const char * esdlDefinitionName, unsigned ver, StringBuffer & esdlServiceName, const char * methodName) = 0;
virtual void addDefinition(IPropertyTree * queryRegistry, const char * name, IPropertyTree *definitionInfo, StringBuffer &newId, unsigned &newSeq, const char *userid, bool deleteprev) = 0;
virtual int publishMethod(const char * espProcName, const char * espBindingName, const char * srvDefId, const char * methodName, IPropertyTree * attributesTree, bool overwrite, StringBuffer & message) = 0;
virtual int publishBinding(const char * bindingName, IPropertyTree * methodsConfig, const char * espProcName, const char * espPort, const char * esdlDefinitionName,

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

Again avoid publish except at the highest level. Maybe "bindService" or "bindESDLService". Maybe combine definitonName and definitionVersion into one "defintionId" for consistency?

This comment has been minimized.

Copy link
@mayx

mayx Jan 4, 2018

Author Member

Changed method name to bindService, and replace name version with definitionId.

virtual int publishMethod(const char * espProcName, const char * espBindingName, const char * srvDefId, const char * methodName, IPropertyTree * attributesTree, bool overwrite, StringBuffer & message) = 0;
virtual int publishBinding(const char * bindingName, IPropertyTree * methodsConfig, const char * espProcName, const char * espPort, const char * esdlDefinitionName,
int esdlDefinitionVersion, const char * esdlServiceName, StringBuffer & message, bool overwrite, const char * user) = 0;
virtual IPropertyTree* getBindingTree(const char * espProcName, const char * espBindingName, StringBuffer & msg) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

This is another one that may evolve as you work on other issues. We really need to support more than one esdl service being bound per port. Also may evolve when not using DALI as back end.

virtual int publishBinding(const char * bindingName, IPropertyTree * methodsConfig, const char * espProcName, const char * espPort, const char * esdlDefinitionName,
int esdlDefinitionVersion, const char * esdlServiceName, StringBuffer & message, bool overwrite, const char * user) = 0;
virtual IPropertyTree* getBindingTree(const char * espProcName, const char * espBindingName, StringBuffer & msg) = 0;
virtual bool deleteDefinition(const char* definitionId, StringBuffer& errmsg, StringBuffer& defxml) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

For an abstract interface seems like returning what you are deleting might not always be needed... so could make it optional... StringBuffer *defxml and only populate it if the pointer is not nullptr.

This comment has been minimized.

Copy link
@mayx

mayx Jan 4, 2018

Author Member

Changed to StringBuffer*

int esdlDefinitionVersion, const char * esdlServiceName, StringBuffer & message, bool overwrite, const char * user) = 0;
virtual IPropertyTree* getBindingTree(const char * espProcName, const char * espBindingName, StringBuffer & msg) = 0;
virtual bool deleteDefinition(const char* definitionId, StringBuffer& errmsg, StringBuffer& defxml) = 0;
virtual bool deleteBinding(const char* bindingId, StringBuffer& errmsg, StringBuffer& bindingxml) = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

Again, optional bindingxml returned.

This comment has been minimized.

Copy link
@mayx

mayx Jan 4, 2018

Author Member

Changed to StringBuffer*

virtual IPropertyTree* getBindingTree(const char * espProcName, const char * espBindingName, StringBuffer & msg) = 0;
virtual bool deleteDefinition(const char* definitionId, StringBuffer& errmsg, StringBuffer& defxml) = 0;
virtual bool deleteBinding(const char* bindingId, StringBuffer& errmsg, StringBuffer& bindingxml) = 0;
virtual IPropertyTree* getDefinitions() = 0;

This comment has been minimized.

Copy link
@afishbeck

afishbeck Dec 29, 2017

Member

These will probably need abstracting later, when moving away from DALI.

@richardkchapman

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

@mayx Back to you

@mayx mayx force-pushed the mayx:HPCC-18408-new branch 3 times, most recently from 776e439 to 218a5ef Jan 4, 2018

@mayx

This comment has been minimized.

Copy link
Member Author

commented Jan 8, 2018

@afishbeck changes made as suggested, please review again.

@afishbeck
Copy link
Member

left a comment

@mayx looks fine

@mayx mayx force-pushed the mayx:HPCC-18408-new branch from 218a5ef to 6710fb7 Feb 12, 2018

@mayx

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

@rpastrana I resolved the merging conflicts and rebased with master, please do a final review...

@rpastrana
Copy link
Member

left a comment

@mayx since most of this had already been reviewed, I focused on ensuring proper integration of HPCC-18881
and HPCC-18905. I confirmed the former, but I didn't see where the latter was integrated. Please confirm.
Also, there were a couple of other comments in line.


return NULL;
}

IPropertyTree * fetchESDLBindingFromStateFile(const char *process, const char *bindingName, const char * stateFileName)

This comment has been minimized.

Copy link
@rpastrana

rpastrana Feb 13, 2018

Member

why don't we move this method to your new class as well?

This comment has been minimized.

Copy link
@mayx

mayx Feb 14, 2018

Author Member

For this pass I'd like to leave the state file out and only deal with "central" repository.

@@ -1096,6 +933,7 @@ EsdlBindingImpl::EsdlBindingImpl()

EsdlBindingImpl::EsdlBindingImpl(IPropertyTree* cfg, const char *binding, const char *process) : CHttpSoapBinding(cfg, binding, process)
{
m_pCentralStore.setown(createEsdlCentralStore());

This comment has been minimized.

Copy link
@rpastrana

rpastrana Feb 13, 2018

Member

does createEsdlCentralStore return a singleton? if not, is there any reason why it can't be?

This comment has been minimized.

Copy link
@mayx

mayx Feb 14, 2018

Author Member

Yes it's already a singleton.

m_esdlBndCfg.set(fetchESDLBinding(process, binding, m_esdlStateFilesLocation));

if (!m_esdlBndCfg.get())
DBGLOG("ESDL Binding: Could not fetch ESDL binding %s for ESP Process %s", binding, process);

//Subscribe to Dali anyway
DBGLOG("ESDL Binding %s is subscribing to all /ESDL/Bindings/Binding dali changes", binding);

This comment has been minimized.

Copy link
@rpastrana

rpastrana Feb 13, 2018

Member

maybe this log entry should be in the centralstore?

This comment has been minimized.

Copy link
@mayx

mayx Feb 14, 2018

Author Member

Removed this log and enhanced corresponding log in central store.

@mayx

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2018

@rpastrana please review again. It seems that 18905 was merged in before, or same changes were made in master, that's why the difference didn't show.

@mayx mayx force-pushed the mayx:HPCC-18408-new branch from 6710fb7 to 53c7a3b Feb 14, 2018

HPCC-18408 Create an interface to hide dali esdl access details and x…
…paths

- Add interface IEsdlStore to abstract esdl dali operations
- Move dali related operations from esdl binding and ws_esdlconfig to
  a class that implements IEsdlStore. Most of the implementation function
  remain unchanged during the move.
- Add IEsdlSubscription and IEsdlListener interfaces to abstract
  subscription and notify operations with dali

Signed-off-by: mayx <yanrui.ma@lexisnexisrisk.com>

@mayx mayx force-pushed the mayx:HPCC-18408-new branch from 53c7a3b to 96dbe3b Feb 26, 2018

@HPCCSmoketest

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

Automated Smoketest:
OS: centos 7.2.1511 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: 96dbe3b
Build: success
Install hpccsystems-platform-community_6.5.0-trunk0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout
unittest 83 83 0 0 0
wutoolTest(Dali) 19 19 0 0 0
wutoolTest(Cassandra) 19 19 0 0 0

Regression test result:

phase total pass fail
setup (hthor) 11 11 0
setup (thor) 11 11 0
setup (roxie) 11 11 0
test (hthor) 761 761 0
test (thor) 675 675 0
test (roxie) 788 788 0

HPCC Stop: OK
HPCC Uninstall: OK
Time stats:

Prep time Build time Package time Install time Start time Test time Stop time Summary
6 sec (00:00:06) 180 sec (00:03:00) 56 sec (00:00:56) 6 sec (00:00:06) 35 sec (00:00:35) 1163 sec (00:19:23) 32 sec (00:00:32) 1478 sec (00:24:38)
@rpastrana
Copy link
Member

left a comment

@mayx looks fine

@richardkchapman richardkchapman merged commit 234826d into hpcc-systems:master Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.