Source Hubspot: check if it has a state on search streams #15110

vladimir-remar · 2022-07-28T16:00:02Z

What

The idea of CRMSearchStream is to use one endpoint or another depending on whether it has a previous state

airbyte/airbyte-integrations/connectors/source-hubspot/source_hubspot/streams.py

Line 761 in 65295b7

    
           return f"/crm/v3/objects/{self.entity}/search" if self.state else f"/crm/v3/objects/{self.entity}"

.
In other words, if it does not have a previous state, use the List endpoint for the CRM object (Contacts, Deals...) and if it has a state, use the search endpoint.

How

Check if has a previous state before setting the state at the beginning of the sync.

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

marcosmarxm · 2022-08-03T20:55:04Z

@grubberr can you check this contirbution I saw you created the other function set_sync for this connector.

davydov-d · 2022-08-09T06:33:38Z

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2823229366
❌ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2823229366
🐛 https://gradle.com/s/khedxyvcdi77k

Build Failed

Test summary info:

Could not find result summary

davydov-d · 2022-08-09T06:57:05Z

hey @vladimir-remar could you please merge the up-to-date master into your branch? otherwise we are not able to run acceptance tests

davydov-d · 2022-08-09T07:13:06Z

airbyte-integrations/connectors/source-hubspot/source_hubspot/streams.py

+    def set_sync(self, sync_mode: SyncMode, stream_state):
+        self._sync_mode = sync_mode
+        if self._sync_mode == SyncMode.incremental:
+            if stream_state:


@vladimir-remar this is the main change here, right?

could you please provide an example of the use case for this? I don't quite get what's the point of this change and under what conditions it would come into action

btw, isn't stream_state value same as self._state?

Hi @davydov-d, thanks for answer.

Indeed it has the same value if the sync mode is incremental and run with a valid state.
Lets get into the main idea I have:

First sync: Incremental / no state, should use f"/crm/v3/objects/{self.entity}"

Following syncs: Incremental / state, should use f"/crm/v3/objects/{self.entity}/search"

Probably I missed something or I got the wrong idea refer to this.
Anyways thanks for your help.

@vladimir-remar thanks for your time.
the reason I'm asking is that unfortunately, this connector's code is quite complicated and this change does not bring in more clarity..

so, if stream_state value is same as self._state, can't it be simplified to:

if self._sync_mode == SyncMode.incremental: if self._state: if not self._state: self._state = self._start_date else: self._state = self._start_date = max(self._state, self._start_date)

?
if so, not branch will never execute. Am I missing something?

@davydov-d Thanks to you.
I am not referring to the current value of the state, but to whether it had a previous initial state, before the first sync,
that is why my suggestion is oriented to whether it is the first sync or not. That's why I identify the initial value of stream_state and then I set the initial value of self._state

It is more related in which endpoint will the stream use

It depends if state property was set before, the actual approach in incremental mode the state will be set always incurring use the /search endpoint.

@vladimir-remar I'm afraid you were misled by this if self._state statement - it's kind of a workaround with potential pitfalls. The idea is to use /search in Incremental mode (or whenever you have a filtering criteria as @grubberr mentioned above) as main url regardless of the state. This code should be refactored I believe 😕

grubberr · 2022-08-09T08:57:25Z

@vladimir-remar

@property
    def url(self):
        return f"/crm/v3/objects/{self.entity}/search" if self.state else f"/crm/v3/objects/{self.entity}"

as I remember we select one or another endpoint depending whether we have filtering parameter or not

if we have filtering param start_date we use /search endpoint
if we don't have filtering param start_date we use /objects endpoint

filtering parameter start_date = max(config, state)

Can you please explain your vision? maybe I am missing something in hubspot API details.

Thank you

vladimir-remar · 2022-08-09T09:37:58Z

as I remember we select one or another endpoint depending whether we have filtering parameter or not

if we have filtering param start_date we use /search endpoint

if we don't have filtering param start_date we use /objects endpoint

filtering parameter start_date = max(config, state)

Hi @grubberr thanks you.
As I see here

airbyte/airbyte-integrations/connectors/source-hubspot/source_hubspot/streams.py

Line 892 in a54c0ef

if self.state:

It depends if state property was set before, the actual approach in incremental mode the state will be set always incurring use the /search endpoint.
If the main intention was use /search in Incremental as main url it makes sense now for me.

davydov-d · 2022-08-11T09:00:46Z

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2838623777
✅ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2838623777
Python tests coverage:

Name                                                 Stmts   Miss  Cover
------------------------------------------------------------------------
source_acceptance_test/utils/__init__.py                 6      0   100%
source_acceptance_test/tests/__init__.py                 4      0   100%
source_acceptance_test/__init__.py                       2      0   100%
source_acceptance_test/tests/test_full_refresh.py       52      2    96%
source_acceptance_test/utils/asserts.py                 37      2    95%
source_acceptance_test/config.py                        82      6    93%
source_acceptance_test/utils/json_schema_helper.py     105     13    88%
source_acceptance_test/tests/test_incremental.py       121     25    79%
source_acceptance_test/utils/common.py                  77     17    78%
source_acceptance_test/tests/test_core.py              355    107    70%
source_acceptance_test/utils/compare.py                 62     23    63%
source_acceptance_test/base.py                          10      4    60%
source_acceptance_test/utils/connector_runner.py       110     48    56%
------------------------------------------------------------------------
TOTAL                                                 1023    247    76%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_hubspot/errors.py          6      0   100%
source_hubspot/constants.py       3      0   100%
source_hubspot/__init__.py        2      0   100%
source_hubspot/helpers.py        70      7    90%
source_hubspot/source.py         90     19    79%
source_hubspot/streams.py       818    199    76%
-------------------------------------------------
TOTAL                           989    225    77%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_hubspot/errors.py          6      0   100%
source_hubspot/constants.py       3      0   100%
source_hubspot/__init__.py        2      0   100%
source_hubspot/helpers.py        70      3    96%
source_hubspot/streams.py       818     76    91%
source_hubspot/source.py         90     18    80%
-------------------------------------------------
TOTAL                           989     97    90%

Build Passed

Test summary info:

All Passed

marcosmarxm · 2022-08-12T12:38:14Z

@davydov-d do you think we can merge this contribution? I can finish it if needed.

davydov-d · 2022-08-12T12:49:01Z

@davydov-d do you think we can merge this contribution? I can finish it if needed.

@marcosmarxm I have strong doubts. Replied to Vladimir above

marcosmarxm · 2022-08-12T12:52:40Z

@davydov-d sorry I missed that,

vladimir-remar · 2022-08-17T10:18:28Z

@davydov-d Thanks.
We are having problems with the synchronization of contacts and this PR would solve them. We have about 314k of records, however the Incremental synchronization is having wrong behaviors resulting in infinite loops or wrong readings of the total results attached log and an image. The current implementation may work fine for a few records but with a lot of records, the Synch takes hours and the cursor gets messed.

We think they would also solve this https://discuss.airbyte.io/t/source-hubspot-contact-list-membership-contacts-extraction-performance-optimization/2219

I say that it is related because the use of the search endpoint causes this behavior.

hubspot_contacts.txt

davydov-d · 2022-08-17T11:33:22Z

@davydov-d Thanks. We are having problems with the synchronization of contacts and this PR would solve them. We have about 314k of records, however the Incremental synchronization is having wrong behaviors resulting in infinite loops or wrong readings of the total results attached log and an image. The current implementation may work fine for a few records but with a lot of records, the Synch takes hours and the cursor gets messed.

We think they would also solve this https://discuss.airbyte.io/t/source-hubspot-contact-list-membership-contacts-extraction-performance-optimization/2219

I say that it is related because the use of the search endpoint causes this behavior. hubspot_contacts.txt

@vladimir-remar are you sure about an infinite loop? the records count and the sync time is probably related to this change.
it reads associations from a specialized endpoint in incremental mode since /search endpoint does not support associations and counts each association as a record. if you dont need those during the sync perhaps we could introduce a new option in the input config or move this sync to another stream.. your thoughts on this are welcome

p.s. could you please try running the sync locally with your change? then we could know for sure if it changes the way sync works

vladimir-remar · 2022-08-18T08:14:03Z

@davydov-d Thanks. We are having problems with the synchronization of contacts and this PR would solve them. We have about 314k of records, however the Incremental synchronization is having wrong behaviors resulting in infinite loops or wrong readings of the total results attached log and an image. The current implementation may work fine for a few records but with a lot of records, the Synch takes hours and the cursor gets messed.
We think they would also solve this https://discuss.airbyte.io/t/source-hubspot-contact-list-membership-contacts-extraction-performance-optimization/2219
I say that it is related because the use of the search endpoint causes this behavior. hubspot_contacts.txt

@vladimir-remar are you sure about an infinite loop? the records count and the sync time is probably related to this change. it reads associations from a specialized endpoint in incremental mode since /search endpoint does not support associations and counts each association as a record. if you dont need those during the sync perhaps we could introduce a new option in the input config or move this sync to another stream.. your thoughts on this are welcome

p.s. could you please try running the sync locally with your change? then we could know for sure if it changes the way sync works

@davydov-d Thanks, I've made a custom connector with my changes, I attached the logs.
hubspot_contacts-first-sync.txt
hubspot_contacts-next-sync.txt

davydov-d · 2022-08-18T09:16:19Z

@davydov-d Thanks. We are having problems with the synchronization of contacts and this PR would solve them. We have about 314k of records, however the Incremental synchronization is having wrong behaviors resulting in infinite loops or wrong readings of the total results attached log and an image. The current implementation may work fine for a few records but with a lot of records, the Synch takes hours and the cursor gets messed.
We think they would also solve this https://discuss.airbyte.io/t/source-hubspot-contact-list-membership-contacts-extraction-performance-optimization/2219
I say that it is related because the use of the search endpoint causes this behavior. hubspot_contacts.txt

@vladimir-remar are you sure about an infinite loop? the records count and the sync time is probably related to this change. it reads associations from a specialized endpoint in incremental mode since /search endpoint does not support associations and counts each association as a record. if you dont need those during the sync perhaps we could introduce a new option in the input config or move this sync to another stream.. your thoughts on this are welcome
p.s. could you please try running the sync locally with your change? then we could know for sure if it changes the way sync works

@davydov-d Thanks, I've made a custom connector with my changes, I attached the logs. hubspot_contacts-first-sync.txt hubspot_contacts-next-sync.txt

@vladimir-remar looks good, makes sense now to accept this change

davydov-d

@marcosmarxm we need to bump version here and update the changelog please

marcosmarxm · 2022-08-22T04:41:19Z

/test connector=connectors/source-hubspot

🕑 connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2901307539
✅ connectors/source-hubspot https://github.com/airbytehq/airbyte/actions/runs/2901307539
Python tests coverage:

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_hubspot/errors.py          6      0   100%
source_hubspot/constants.py       3      0   100%
source_hubspot/__init__.py        2      0   100%
source_hubspot/helpers.py        70      7    90%
source_hubspot/source.py         90     19    79%
source_hubspot/streams.py       818    198    76%
-------------------------------------------------
TOTAL                           989    224    77%
Name                          Stmts   Miss  Cover
-------------------------------------------------
source_hubspot/errors.py          6      0   100%
source_hubspot/constants.py       3      0   100%
source_hubspot/__init__.py        2      0   100%
source_hubspot/helpers.py        70      3    96%
source_hubspot/streams.py       818     76    91%
source_hubspot/source.py         90     18    80%
-------------------------------------------------
TOTAL                           989     97    90%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          10      4    60%   15-18
	 source_acceptance_test/config.py                        83      6    93%   78-80, 84-86
	 source_acceptance_test/conftest.py                     164    164     0%   6-282
	 source_acceptance_test/plugin.py                        48     48     0%   6-104
	 source_acceptance_test/tests/test_core.py              329    111    66%   39, 50-58, 63-70, 74-75, 79-80, 164, 202-219, 228-236, 240-245, 251, 284-289, 327-334, 374-376, 379, 439-448, 477-478, 484, 487, 520-530, 543-568, 573-577
	 source_acceptance_test/tests/test_full_refresh.py       52      2    96%   34, 65
	 source_acceptance_test/tests/test_incremental.py       121     25    79%   21-23, 29-31, 36-43, 48-61, 208-216
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  77     17    78%   15-16, 24-30, 47-54, 64, 67
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       110     48    56%   23-26, 32, 36, 39-64, 67-69, 72-74, 77-79, 82-84, 87-89, 92-110, 144-146
	 source_acceptance_test/utils/json_schema_helper.py     105     13    88%   30-31, 38, 41, 65-68, 96, 120, 190-192
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1321    463    65%

Build Passed

Test summary info:

All Passed

marcosmarxm · 2022-08-22T05:03:17Z

/publish connector=connectors/source-hubspot

🕑 Publishing the following connectors:
connectors/source-hubspot
https://github.com/airbytehq/airbyte/actions/runs/2901378979

Connector	Did it publish?	Were definitions generated?
connectors/source-hubspot	✅	✅

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

* check if it has a state * update version in Dockerfile and docs * auto-bump connector version [ci skip] Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>

check if it has a state

fbb79a7

octavia-squidington-iii added bounty community connectors/source/hubspot labels Jul 28, 2022

github-actions bot added the area/connectors Connector related issues label Jul 28, 2022

marcosmarxm requested review from lazebnyi and grubberr August 3, 2022 20:51

sajarin added the bounty-S Maintainer program: claimable small bounty PR label Aug 5, 2022

davydov-d reviewed Aug 9, 2022

View reviewed changes

Merge branch 'master' into source_hubspot_check_state_on_search_streams

0ed840d

vladimir-remar requested a review from davydov-d August 11, 2022 07:36

sajarin added internal and removed bounty bounty-S Maintainer program: claimable small bounty PR labels Aug 11, 2022

davydov-d approved these changes Aug 18, 2022

View reviewed changes

vladimir-remar added 2 commits August 18, 2022 11:55

Merge branch 'master' into source_hubspot_check_state_on_search_streams

d7b59ae

update version in Dockerfile and docs

682f402

github-actions bot added the area/documentation Improvements or additions to documentation label Aug 18, 2022

lazebnyi removed their request for review August 19, 2022 02:57

auto-bump connector version [ci skip]

2065e12

marcosmarxm merged commit eb2c7ad into airbytehq:master Aug 22, 2022

evantahler mentioned this pull request Aug 22, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1 #15857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source Hubspot: check if it has a state on search streams #15110

Source Hubspot: check if it has a state on search streams #15110

vladimir-remar commented Jul 28, 2022

marcosmarxm commented Aug 3, 2022

davydov-d commented Aug 9, 2022 •

edited by github-actions bot

Loading

davydov-d commented Aug 9, 2022

davydov-d Aug 9, 2022

vladimir-remar Aug 9, 2022 •

edited

Loading

davydov-d Aug 11, 2022

vladimir-remar Aug 11, 2022

davydov-d Aug 12, 2022

grubberr commented Aug 9, 2022

vladimir-remar commented Aug 9, 2022

davydov-d commented Aug 11, 2022 •

edited by github-actions bot

Loading

marcosmarxm commented Aug 12, 2022

davydov-d commented Aug 12, 2022

marcosmarxm commented Aug 12, 2022

vladimir-remar commented Aug 17, 2022

davydov-d commented Aug 17, 2022 •

edited

Loading

vladimir-remar commented Aug 18, 2022

davydov-d commented Aug 18, 2022

davydov-d left a comment

marcosmarxm commented Aug 22, 2022 •

edited by github-actions bot

Loading

marcosmarxm commented Aug 22, 2022 •

edited by github-actions bot

Loading

Source Hubspot: check if it has a state on search streams #15110

Source Hubspot: check if it has a state on search streams #15110

Conversation

vladimir-remar commented Jul 28, 2022

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Tests

marcosmarxm commented Aug 3, 2022

davydov-d commented Aug 9, 2022 • edited by github-actions bot Loading

Build Failed

davydov-d commented Aug 9, 2022

davydov-d Aug 9, 2022

Choose a reason for hiding this comment

vladimir-remar Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

davydov-d Aug 11, 2022

Choose a reason for hiding this comment

vladimir-remar Aug 11, 2022

Choose a reason for hiding this comment

davydov-d Aug 12, 2022

Choose a reason for hiding this comment

grubberr commented Aug 9, 2022

vladimir-remar commented Aug 9, 2022

davydov-d commented Aug 11, 2022 • edited by github-actions bot Loading

Build Passed

marcosmarxm commented Aug 12, 2022

davydov-d commented Aug 12, 2022

marcosmarxm commented Aug 12, 2022

vladimir-remar commented Aug 17, 2022

davydov-d commented Aug 17, 2022 • edited Loading

vladimir-remar commented Aug 18, 2022

davydov-d commented Aug 18, 2022

davydov-d left a comment

Choose a reason for hiding this comment

marcosmarxm commented Aug 22, 2022 • edited by github-actions bot Loading

Build Passed

marcosmarxm commented Aug 22, 2022 • edited by github-actions bot Loading

davydov-d commented Aug 9, 2022 •

edited by github-actions bot

Loading

vladimir-remar Aug 9, 2022 •

edited

Loading

davydov-d commented Aug 11, 2022 •

edited by github-actions bot

Loading

davydov-d commented Aug 17, 2022 •

edited

Loading

marcosmarxm commented Aug 22, 2022 •

edited by github-actions bot

Loading

marcosmarxm commented Aug 22, 2022 •

edited by github-actions bot

Loading