[SPARK-26371][SS] Increase kafka ConfigUpdater test coverage.

[gaborgsomogyi]

What changes were proposed in this pull request?

As Kafka delegation token added logic into ConfigUpdater it would be good to test it.
This PR contains the following changes:

• ConfigUpdater extracted to a separate file and renamed to KafkaConfigUpdater
• mockito-core dependency added to kafka-0-10-sql
• Unit tests added

How was this patch tested?

Existing + new unit tests + on cluster.

[gaborgsomogyi]

cc @HeartSaVioR @vanzin

[SparkQA]

Test build #100147 has finished for PR 23321 at commit 7026056.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

I don't know the details of the tests here but code looks good and more test coverage is good.

[gaborgsomogyi]

retest this, please

[dongjoon-hyun]

@gaborgsomogyi . Let's not use [TESTS] when you touch src/main. Also, this PR is a kind of refactoring PR which moves ConfigUpdater to KafkaConfigUpdater, isn't it.

[gaborgsomogyi]

@dongjoon-hyun removed. Good to know with [TESTS] not even rename is allowed. Next time I'll stick to that.

[dongjoon-hyun]

Thank you for updating, @gaborgsomogyi . Yep, sometimes, it's used in the mixed situations. But, in general, [TESTS] means src/test-only PR.

[dongjoon-hyun]

Just a question. So, setAuthenticationConfigIfNeeded is currently no-op in this case intentionally. Is it better to raise an IllegalArgumentException with some directional error message about requirements?

[HeartSaVioR]

It seems better to me too to let setAuthenticationConfigIfNeeded only works properly with security config. (need to remove IfNeeded then) It would fail fast and gives us the chance to provide proper guide, maybe missing configuration for authentication.

But no strong opinion since this looks like providing convenience vs being strict to help end users to find possible missing point, and both look reasonable.

[gaborgsomogyi]

This was the original implementation which was asked to change like this on other PRs (and I agree with it).
The reasoning behind was to keep fluent API and not having something like this(which is hardly testable):

    val updater = KafkaConfigUpdater("executor", specifiedKafkaParams)
      .set(...)
      .set(...)

    if (!globalSecurity && tokenAvailable)
      updater.setAuthenticationConfig(...)
    else if (somethingNew)
      updater.set(...)

    updater.build()

There is no flag like kafka.securityEnabled and Kafka can't tell it either.

Is it better to raise an IllegalArgumentException with some directional error message about requirements?

Security parameters shouldn't always set, for example:

• If unsecure
• If user provided JVM global JAAS configuration (this provide total freedom to users)

It would fail fast and gives us the chance to provide proper guide

• No security => nothing to check and do
• With global JAAS => it's not possible to parse JAAS config and suggest changes
• With DT => The user warned in advance to provide SCRAM related sasl.mechanism because the related Kafka error message is cryptic a bit.

I personally don't see more possibilities to provide convenience for users.

[HeartSaVioR]

I meant the case end users intend to set security so call setAuthenticationConfig* in KafkaConfigUpdater but they missed setting up security config. I guess error message will be provided in Kafka side but it might be cryptic so seeking the possibility to provide better message.
No strong opinion and either is fine because end users can be indicated in any way. Just for clarifying my opinion.

[SparkQA]

Test build #100154 has finished for PR 23321 at commit 7026056.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #100160 has finished for PR 23321 at commit 2a7a18e.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[HeartSaVioR]

Looks good overall. Left some comments but actually these are covered by @dongjoon-hyun review, and looks like nits.

[HeartSaVioR]

It seems better to me too to let setAuthenticationConfigIfNeeded only works properly with security config. (need to remove IfNeeded then) It would fail fast and gives us the chance to provide proper guide, maybe missing configuration for authentication.

But no strong opinion since this looks like providing convenience vs being strict to help end users to find possible missing point, and both look reasonable.

[srowen]

This seems OK to me as-is.

[SparkQA]

Test build #100186 has finished for PR 23321 at commit 0347cb6.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #100234 has finished for PR 23321 at commit dcacd75.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

Thank you, @gaborgsomogyi , @srowen , and @HeartSaVioR .

+1, LGTM. Merged to master.

[gaborgsomogyi]

Thanks everybody to make this better, especially @dongjoon-hyun by taking care of the merge.