phet commented on code in PR #3773:
URL: https://github.com/apache/gobblin/pull/3773#discussion_r1333962323
##########
gobblin-salesforce/src/main/java/org/apache/gobblin/salesforce/SalesforceSource.java:
##########
@@ -462,9 +459,9 @@ private static Set<SourceEntity> getSourceEntities(String
response) {
}
protected SalesforceConnector getConnector(State state) {
- if (this.salesforceConnector == null) {
- this.salesforceConnector = new SalesforceConnector(state);
+ if (salesforceConnector == null) {
+ salesforceConnector = new SalesforceConnector(state);
Review Comment:
this is not thread-safe... should it be `synchronized`? (if there's any
need to explicitly close the connector, there's a potential resource leak.)
##########
gobblin-salesforce/src/main/java/org/apache/gobblin/salesforce/SalesforceSource.java:
##########
@@ -133,11 +130,11 @@ protected void addLineageSourceInfo(SourceState
sourceState, SourceEntity entity
}
@Override
protected List<WorkUnit> generateWorkUnits(SourceEntity sourceEntity,
SourceState state, long previousWatermark) {
- SalesforceConnector connector = getConnector(state);
SfConfig sfConfig = new SfConfig(state.getProperties());
- if (salesforceHistogramService == null) {
- salesforceHistogramService = new SalesforceHistogramService(sfConfig,
connector);
+ if (histogramService == null) {
+ salesforceConnector = getConnector(state);
+ histogramService = new RecordModTimeHistogramService(sfConfig,
getConnector(state));
Review Comment:
maybe add comment on why another `getConnector(state)`, rather than reusing
edit: reading on I see it doesn't create a new connector... just looks that
way
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
