Fixes to Source Job Connector and Master Monitor Logging #703
Conversation
kmg-stripe
force-pushed
the
kmg-small-fixes
branch
from
6f44b3f
to
72bdc53
Compare
1. All source job connectors have ZK config hard-coded due to parameter-less MantisSourceJobConnectorFactory. Configuring a MantisClient in this context doesn't make sense, since we can reference the configured HighAvailabilityServices 2. DynamoDBMasterMonitor was erroneuously logging parse errors when reading leadership info from the DB. 3. HighAvailabilityServicesImpl was setting the leader state to NULL, which lead to ResourceClusterGatewayClient constructing an invalid URI. Updating to not update the ResourceClusterGatewayClient when master is NULL.
kmg-stripe
force-pushed
the
kmg-small-fixes
branch
from
72bdc53
to
1bda05c
Compare
| public Builder(Properties properties) { | ||
| this(new MantisClient(properties)); | ||
| } | ||
|
|
||
| public Builder() { |
There was a problem hiding this comment.
We were seeing log spam, where a JobSource triggers this code path. It doesn't appear to break anything, but seems a bit weird and leads to log spam. HighAvailabilityServicesUtil logs a few warnings (i.e. "HA service running in local mode. This is only valid in local test."), because this path attempts to "create" the HA services with no properties. I think it is a little weird because I wouldn't expect a job writer to pass Mantis platform configuration for discovering the master, so it seems best to use HighAvailabilityServicesUtil. That is, unless I am missing something...
My understanding is that the HA services should generally be configured if we are here, so should just try to get the reference to HA services.
kmg-stripe
requested review from
calvin681,
sundargates,
Andyz26,
hmitnflx and
fdc-ntflx
as code owners
|
cc: @codyrioux |
|
@Andyz26 - Just a ping... I was also hoping to cut a release after this one. |
| @@ -113,6 +113,12 @@ public MantisClient(MasterClientWrapper clientWrapper, boolean disablePingFilter | |||
| this.clientWrapper = clientWrapper; | |||
| } | |||
|
|
|||
| public MantisClient(HighAvailabilityServices haServices) { | |||
There was a problem hiding this comment.
I would suggest adding a javadoc here to let user know this will start running the HAService. Also in this case who should be closing the haService if it creates other threads?
| } | ||
|
|
||
| public MantisSourceJobConnector() { | ||
| props = new Properties(); | ||
| // todo(kmg-stripe): Can we remove this? It seems it is only used by main in this class for testing. |
There was a problem hiding this comment.
feel free to remove this (or move it to test).
kmg-stripe
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
All source job connectors have ZK config hard-coded due to parameter-less
MantisSourceJobConnectorFactory. Configuring aMantisClientin this contextdoesn't make sense, since we can reference the configured
HighAvailabilityServicesDynamoDBMasterMonitorwas erroneously logging parse errors when readingleadership info from the DB.
HighAvailabilityServicesImplwas setting the leader state to NULL, which leadto
ResourceClusterGatewayClientconstructing an invalid URI. Updating to notupdate the
ResourceClusterGatewayClientwhen master is NULL.Context
Explain context and other details for this pull request.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all tests