Skip to content
This repository has been archived by the owner on Apr 19, 2022. It is now read-only.

Handle hdfs_namenode_principal properly and refactor auto configuration logic #192

Merged
merged 1 commit into from
Feb 25, 2016

Conversation

garthcn
Copy link
Contributor

@garthcn garthcn commented Feb 19, 2016

This is related to this issue: #175

See this comment: #175 (comment)

This might not be the ideal way since removing the side effects here and here might be a better solution.

Let me know if this pull request makes sense and really appreciate your great work on the library!

@garthcn garthcn changed the title Add the option to pass HDFS namenode principal to client constructors [HOLD] Add the option to pass HDFS namenode principal to client constructors Feb 23, 2016
@garthcn garthcn changed the title [HOLD] Add the option to pass HDFS namenode principal to client constructors Add the option to pass HDFS namenode principal to client constructors Feb 23, 2016
@garthcn garthcn changed the title Add the option to pass HDFS namenode principal to client constructors Handle hdfs_namenode_principal properly and refactor auto configuration logic Feb 23, 2016
@garthcn
Copy link
Contributor Author

garthcn commented Feb 24, 2016

More detailed description:

The problem: #175 (comment)

Changes in the PR:

  1. Remove class variables in HDFSConfig. Calling get_external_config() will return a dict like this:

    {
        namenodes: [{'namenodes': <host>, 'port': 1234}],
        use_trash: True,
        use_sasl: True,
        hdfs_namenode_principal: <principal>
    }
    
  2. In places where the class variables in HDFSConfig were referenced, call get_external_config() and use the values returned in the dict.

  3. In Client, HAClient, add the option to pass in hdfs_namenode_principal. AutoConfigClient will call get_external_config() and pass hdfs_namenode_principal to HAClient.

  4. In SocketRpcChannel, SaslRpcClient and RpcService, change __init__ to accept the hdfs_namenode_principal which will be passed down the chain from Client or HAClient.

  5. Update test cases to reflect the change

@wouterdebie
Copy link

I'll talk to @ravwojdyla tomorrow, but LGTM for now.

@wouterdebie
Copy link

Would you be able to squash this commit?

…on logic

Changes in the PR:
1. Remove class variables in `HDFSConfig`. Calling `get_external_config()` will return a dict like this:

    ```
    {
        namenodes: [{'namenodes': <host>, 'port': 1234}],
        use_trash: True,
        use_sasl: True,
        hdfs_namenode_principal: <principal>
    }
    ```
2. In places where the class variables in `HDFSConfig` were referenced, call  `get_external_config()` and use the values returned in the dict.

3. In `Client`, `HAClient`, add the option to pass in `hdfs_namenode_principal`. `AutoConfigClient` will call `get_external_config()` and pass `hdfs_namenode_principal` to `HAClient`.

4. In `SocketRpcChannel`, `SaslRpcClient` and `RpcService`, change `__init__` to accept the `hdfs_namenode_principal` which will be passed down the chain from `Client` or `HAClient`.

5. Update test cases to reflect the change
@garthcn
Copy link
Contributor Author

garthcn commented Feb 24, 2016

@wouterdebie I just squashed the commits into one. Let me know if you have any comments/feedback. Thanks!

@ravwojdyla
Copy link
Contributor

We eventually want to refactor the way we handle config/ctor, but that is out of the scope of this bug/PR. Thanks @garthcn for great investigation and patch. lgtm.

ravwojdyla added a commit that referenced this pull request Feb 25, 2016
Handle hdfs_namenode_principal properly and refactor auto configuration logic
@ravwojdyla ravwojdyla merged commit 214c307 into spotify:master Feb 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants