[idbroker] Refactor IDBroker with improved HA support and giving more preference to RAZ when both are configured in Hue #3626
Conversation
Harshg999
changed the title
|
@Harshg999 , what is the numbering 42,50,52 is there a specific need to add the in the debug statements? The rest looks fine but there seem to be failures with LOG.debug. Please check and fix it. |
|
This was more for code patching in the repro cluster to check what is the root cause for Hue's dependence on IDBroker in RAZ enabled env. I still have to test out this live. |
Harshg999
force-pushed
the
idbroker-check
branch
3 times, most recently
from
32fa5dd
to
1c6c54b
Compare
Harshg999
changed the title
Harshg999
changed the title
Harshg999
force-pushed
the
idbroker-check
branch
from
cde3a48
to
1791175
Compare
Harshg999
changed the title
Harshg999
requested review from
amitsrivastava,
JohanAhlen,
athithyaaselvam,
agl29,
wing2fly and
ranade1
Harshg999
force-pushed
the
idbroker-check
branch
from
d09af34
to
1be02d8
Compare
| @@ -89,7 +82,12 @@ def get_cab_password(fs=None): | |||
| def is_idbroker_enabled(fs=None): | |||
| from desktop.conf import RAZ # Must be imported dynamically in order to have proper value | |||
|
|
|||
| return get_cab_address(fs) is not None and not RAZ.IS_ENABLED.get() # Skipping IDBroker for FS when RAZ is present | |||
| fs = validate_fs(fs) | |||
There was a problem hiding this comment.
so when you are checking the validation of filesystems here it will only give warning in logs but looks like we need to implement like ?
idbroker_addr_from_coresite = get_conf().get(_CNF_CAB_ADDRESS % fs) if fs else None
There was a problem hiding this comment.
For fs = None scenario, validate_fs(fs) will also return None
And then it will try to find the respective config value in this line: idbroker_addr_from_coresite = get_conf().get(_CNF_CAB_ADDRESS % fs)
So get_conf().get('fs.None.ext.cab.address') will also return None because there is no such config.
| try: | ||
| response = requests.get(idb.rstrip('/') + '/dt/knoxtoken/api/v1/token', auth=HTTPKerberosAuth(), verify=False) | ||
| except Exception as e: | ||
| if 'Failed to establish a new connection' in str(e): |
There was a problem hiding this comment.
Should we not handle Name or service not known as this could be a different one with failed to establish the connection. Adding this information should also help
There was a problem hiding this comment.
I logged the response for both failure cases: When the whole host was down from AWS console and when the IDBroker instances were down from Data Lake CM.
In both cases, I didn't see Name or service not known string in response but Failed to establish a new connection was present both times.
I think because of the former only old implementation was not working as expected and then I thought that why even check for it when its flaky and only depend on the stuff which was coming in response both times? I used Failed to establish a new connection in implementing other service HA also like Knox JWT and RAZ recently.
What do you think?
… giving more preference to RAZ when both are configured in Hue (#3626) What changes were proposed in this pull request? - Refactor IDBroker support and give more preference to RAZ when both are configured in Hue. - Improved IDBroker HA code section to switchover to healthy instance correctly and not depend only on the first one for every scenario. This should improve Hue page loading performance also. How was this patch tested? - Tested manually in a live E2E setup with RAZ enabled to check for no regressions + correct IDBroker switchover + improved Hue page load time. - Update existing unit tests. - Adding new unit tests for IDBroker HA. (cherry picked from commit 0e711ea) (cherry picked from commit 1a25a14f62f0433d79dff251beed75ca148dc2e7) Change-Id: I1abf2a61823ccb9a87161df51d2b2e0afae266bd (cherry picked from commit 2233672201dc74401b21971079b2c0872e9d75cf)
What changes were proposed in this pull request?
How was this patch tested?