CDAP-15237 Enhance the streaming plugin to support CDC option #14

mlozbin-cybervisiontech · 2019-05-03T20:49:04Z

JIRA: https://issues.cask.co/browse/CDAP-15237
WIKI: https://wiki.cask.co/display/CE/CDC+Salesforce+Streaming+Source

In scope of this PR:

source plugin implementation
widget
plugin docs

mlozbin-cybervisiontech · 2019-05-03T20:49:33Z

@albertshau could you, please, take a look

albertshau

only partially reviewed. Please add much more documentation around what is happening. I'm having a hard time following the logic.

albertshau · 2019-05-03T23:29:57Z

docs/CDCSalesforce-streamingsource.md

+
+Properties
+----------
+**clientId**: Client ID from the connected app


These should match the user friendly names set in the widget spec that the user will see in the UI. Client ID, Client Secret, etc.

albertshau · 2019-05-03T23:30:20Z

docs/CDCSalesforce-streamingsource.md

+
+**clientSecret**: Client Secret from the connected app
+
+**username**: Username


Username to use when connecting to Salesforce

albertshau · 2019-05-03T23:30:28Z

docs/CDCSalesforce-streamingsource.md

+
+**username**: Username
+
+**password**: Password


Password to use when connecting to Salesforce

albertshau · 2019-05-03T23:31:11Z

docs/CDCSalesforce-streamingsource.md

+
+**password**: Password
+
+**loginUrl**: (default is https://login.salesforce.com/services/oauth2/token) For Salesforce sandbox runs login url is 


Salesforce login URL to authenticate against. The default value is https://login.salesforce.com/services/oauth2/token. This should be changed when running against the Salesforce sandbox.

albertshau · 2019-05-03T23:32:22Z

docs/CDCSalesforce-streamingsource.md

+**loginUrl**: (default is https://login.salesforce.com/services/oauth2/token) For Salesforce sandbox runs login url is 
+different. That's why user needs this option.
+
+**objects**: list of object's API names (For example: Task for base object and Employee__c for custom) separated by ",".


Objects to read change events from.

albertshau · 2019-05-03T23:47:06Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceEventTopicListener.java

+
+  public SalesforceEventTopicListener(AuthenticatorCredentials credentials, List<String> objectsForTracking) {
+    this.credentials = credentials;
+    this.objectsForTracking = objectsForTracking;


it's better to create a copy of the input list, in case it gets modified by the caller.

albertshau · 2019-05-03T23:48:20Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceEventTopicListener.java

+   * Start the Bayeux Client which listens to the Salesforce EventTopic and saves received messages
+   * to the queue.
+   */
+  public void start() {


seems like there should be a corresponding stop() method.

albertshau · 2019-05-03T23:49:35Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+          case STOP:
+            throw new RuntimeException("Failed to process message", e);
+          default:
+            throw new UnexpectedFormatException(String.format("Unknown error handling strategy '%s'", errorHandling));


we usually use IllegalStateException for these cases in enums that shouldn't even happen

albertshau · 2019-05-04T00:04:39Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+
+/**
+ * Implementation of Spark receiver to receive Salesforce change events
+ */


please add more javadocs about what this class is doing. More specifically, how it consumes the change events and processes them.

There needs to be far more documentation here. What are the different types of events that can be read from Salesforce? What are the types of events that this emits? Examples?

albertshau · 2019-05-04T00:12:19Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/records/ChangeEventRecord.java

+/**
+ * Contains information about change event
+ */
+public class ChangeEventRecord {


There is no salesforce class for this objects returned by their API?

If there isn't, we should use Gson in combination with a Java class that directly represents the JSON response for the API, instead of doing custom parsing.

nitinmotgi

Minor comments

nitinmotgi · 2019-05-04T00:35:54Z

widgets/CDCSalesforce-streamingsource.json

+        "type": "record",
+        "fields": [
+          {
+            "name": "cdcMessage",


Can name the field ‘cdc_msg’

nitinmotgi · 2019-05-04T00:36:32Z

widgets/CDCSalesforce-streamingsource.json

+        },
+        {
+          "widget-type": "dsv",
+          "label": "Objects for tracking",


“Tracking Objects”

nitinmotgi · 2019-05-04T00:37:10Z

widgets/CDCSalesforce-streamingsource.json

+          "name": "clientSecret"
+        },
+        {
+          "widget-type": "textbox",


Shouldn’t this be first ?

mlozbin-cybervisiontech · 2019-05-09T23:21:06Z

@albertshau @nitinmotgi addressed all your comments

albertshau

Let's pause work on this. I think there are large gaps in the design and understanding of the API, and I am thinking CDC in general needs a complete re-design.

albertshau · 2019-05-09T23:45:05Z

docs/CDCSalesforce-streamingsource.md

+
+Properties
+----------
+**Client Id**: Client ID from the connected app.


There was another PR in the salesforce project that renamed these (https://github.com/data-integrations/salesforce/blob/master/docs/Salesforce-batchsource.md). Let's use the same wording and descriptions.

albertshau · 2019-05-09T23:49:00Z

docs/CDCSalesforce-streamingsource.md

+
+Salesforce Change Data Capture
+--------------------------
+When something changes in object for which is enable 'Change notifications'. A Change Data Capture event, or change 


All of this information seems like it should be moved from here to the Description section.

albertshau · 2019-05-09T23:49:29Z

docs/CDCSalesforce-streamingsource.md

+
+Salesforce Change Data Capture
+--------------------------
+When something changes in object for which is enable 'Change notifications'. A Change Data Capture event, or change 


Remove "When something changes in object for which is enable 'Change notifications'."

albertshau · 2019-05-09T23:54:07Z

docs/CDCSalesforce-streamingsource.md

+**Tracking Objects**: Objects to read change events from (For example: Task for base object and Employee__c for custom) separated by ",".
+If list is empty then subscription for all events will be used.
+
+**Error Handling**: Possible values are: "Skip on error" or "Fail on error". These are strategies on handling records 


When would a user ever want to skip a change event? Skipping an event can cause the application of future events to fail and kind of defeats the purpose of CDC.

albertshau · 2019-05-09T23:56:21Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+            events.put(event.getTransactionKey(), eventsList);
+          }
+        }
+      } catch (Exception e) {


what are the different types of exceptions that can happen? Seems like it will need to behave differently depending on the type of error scenario.

In general, this should retry whenever it encounters a failure. For example, suppose salesforce is temporarily down. The source should retry until the API comes back up. It should also not flood the logs in that type of situation, but log every X seconds.

albertshau · 2019-05-10T00:09:50Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/records/ChangeEventHeader.java

+
+/**
+ * Contains information about change event. Should be used instead of {@link com.sforce.soap.partner.ChangeEventHeader}
+ * because GSON does not support setters.


I don't understand what this means. Why would we want setters? Why does this class have setters?

albertshau · 2019-05-10T00:14:28Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+  private void processEvents(List<ChangeEventHeader> events, PartnerConnection connection) throws ConnectionException {
+    for (ChangeEventHeader event : events) {
+      SObjectDescriptor descriptor = SObjectDescriptor.fromName(event.getEntityName(), connection);
+      SObjectsDescribeResult describeResult = new SObjectsDescribeResult(connection, descriptor.getAllParentObjects());


is this looking up the SObject descriptor as it currently is? Does the change event not have all the information required to apply the change? If not, I don't understand how you can apply the right change (or how you can do anything efficiently), as this object may be different now than it was when the change event occurred. Unless objects cannot change?

I thought I asked these questions in the design doc. Were they ever addressed?

For CREATE, DELETE, UNDELETE, UPDATE types change event message contains information only about fields which were changed. If type of event is GAP_CREATE, GAP_DELETE, GAP_UNDELETE, GAP_UPDATE, GAP_OVERFLOW than it will contain only ids of records. So we have to make requests to salesforce to get full information.

albertshau · 2019-05-10T00:16:26Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+      Schema schema = SalesforceRecord.getSchema(descriptor, describeResult);
+      updateSchemaIfNecessary(event.getEntityName(), schema);
+
+      if (getOperationType(event) != OperationType.DELETE) {


it's more natural to read if you do:

if (getOperationType(event) == OperationType.DELETE) { sendDeleteRecords() } else { sendUpdateRecords() }

albertshau · 2019-05-10T00:18:21Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+
+/**
+ * Implementation of Spark receiver to receive Salesforce change events
+ */


There needs to be far more documentation here. What are the different types of events that can be read from Salesforce? What are the types of events that this emits? Examples?

albertshau · 2019-05-10T00:21:40Z

src/main/java/io/cdap/plugin/cdc/source/salesforce/SalesforceReceiver.java

+  private void sendUpdateRecords(ChangeEventHeader event, SObjectDescriptor descriptor, Schema schema,
+                                 PartnerConnection connection) throws ConnectionException {
+    String query = getQuery(event, descriptor.getFieldsNames());
+    QueryResult queryResult = connection.query(query);


This is querying the SObject to get the record value?? Maybe I am missing some key information about the salesforce API.

What happens when 2 records with the same key get updated in an SObject? Do we get 2 change events? This type of information needs to be documented.

If update is similar for several records of the same type, we can receive the list of ids that were changed and common changes of their fields.

CDAP-15237 Enhance the streaming plugin to support CDC option

a0da644

albertshau reviewed May 4, 2019

View reviewed changes

nitinmotgi reviewed May 4, 2019

View reviewed changes

CDAP-15237 Enhance the streaming plugin to support CDC option

a0ce65c

albertshau reviewed May 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CDAP-15237 Enhance the streaming plugin to support CDC option #14

CDAP-15237 Enhance the streaming plugin to support CDC option #14

mlozbin-cybervisiontech commented May 3, 2019

mlozbin-cybervisiontech commented May 3, 2019

albertshau left a comment

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 3, 2019

albertshau May 4, 2019

albertshau May 10, 2019

albertshau May 4, 2019

nitinmotgi left a comment

nitinmotgi May 4, 2019

nitinmotgi May 4, 2019

nitinmotgi May 4, 2019

mlozbin-cybervisiontech commented May 9, 2019

albertshau left a comment

albertshau May 9, 2019

albertshau May 9, 2019

albertshau May 9, 2019

albertshau May 9, 2019

albertshau May 9, 2019

albertshau May 10, 2019

albertshau May 10, 2019

mlozbin-cybervisiontech May 17, 2019

albertshau May 10, 2019

albertshau May 10, 2019

albertshau May 10, 2019

mlozbin-cybervisiontech May 17, 2019


		clientSecret: Client Secret from the connected app

		username: Username


		password: Password

		loginUrl: (default is https://login.salesforce.com/services/oauth2/token) For Salesforce sandbox runs login url is

CDAP-15237 Enhance the streaming plugin to support CDC option #14

Are you sure you want to change the base?

CDAP-15237 Enhance the streaming plugin to support CDC option #14

Conversation

mlozbin-cybervisiontech commented May 3, 2019

mlozbin-cybervisiontech commented May 3, 2019

albertshau 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

nitinmotgi 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

mlozbin-cybervisiontech commented May 9, 2019

albertshau 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