[Kernel] [CC Refactor #3] Add ConfigurationProvider and AbstractCommitCoordinatorBuilder APIs

[scottsand-db]

Which Delta project/connector is this regarding?

• Spark
• Standalone
• Flink
• Kernel
• Other (fill in here)

Description

Adds new ConfigurationProvider and AbstractCommitCoordinatorBuilder APIs

How was this patch tested?

Tests are added in CC Refactor PR 5: #3821

Does this PR introduce any user-facing changes?

Yes see the above

[sumeet-db]

We also have CoordinatedCommitUtils.java

Would be nice to either merge them in the same file eventually or have better names for them

[scottsand-db]

Eventually, everything Coordinated-Commits-related in delta-storage will be refactored and moved to delta-kernel, and then be deleted.

[LukasRupprecht]

Do we need to make them implement ConfigurationProvider or how can we, e.g., pass a SparkConf as a ConfigurationProvider?

[scottsand-db]

They need to pass in something that is a ConfigurationProvider (i.e. that implements the interface). You can easily create a wrapper around a SparkConf or a HadoopConf to implemeent the ConfigurationProvider interface

[scottsand-db]

@LukasRupprecht and @sumeet-db -- is it every possible or intended/desired that AbstractMetadata and AbstractProtocol would be null? or are these required params? I see null being passed in in some existing tests today ...

delta/kernel/kernel-api/src/main/java/io/delta/kernel/coordinatedcommits/CommitCoordinatorClient.java

Line 83 in 6ae4b62

AbstractMetadata currentMetadata,

[sumeet-db]

What if you need both hadoopConf and sparkConf?

[scottsand-db]

Great question. You can union them to implement the get interface.

When we go to implement the UC CCC using this API, we can adjust methods as needed.

[sumeet-db]

If I understand correctly, the dev experience for using this method will be:

CommitCoordinatorClient client = AbstractCommitCoordinatorBuilder
                .buildCommitCoordinatorClient(
                    "dynamodb",
                    sessionConfig,
                    coordinatorConf
                );

Somehow, it doesn't feel intuitive that this code will look up the builder in the conf and do stuff

[sumeet-db]

How's this method different than CommitCoordinatorProvider.getBuilder(... /* load builder dynamically from conf */).build(...)? And why do you prefer this one? 🤔

[scottsand-db]

https://github.com/delta-io/delta/blob/master/kernel/kernel-defaults/src/main/java/io/delta/kernel/defaults/internal/coordinatedcommits/CommitCoordinatorProvider.java

is the old code that will be deleted when this is refactored. it uses HadoopConf and io.delta.storage APIs

[scottsand-db]

Somehow, it doesn't feel intuitive that this code will look up the builder in the conf and do stuff

Really? I quite like it. Feel free to let me know what doesn't feel right about this. This is what we agreed on in the sync on Tuesday Oct 22 @ 1:00pm

[scottsand-db]

Further, this API feels very similar to the previous API of

CommitCoordinatorClient client = CommitCoordinatorProvider
  .getCommitCoordinatorClient(
    hadoopConf,
    name,
    commitCoordinatorConf);

[sumeet-db]

I was hoping we could prevent patterns like the following by moving buildCommitCoordinatorClient to a separate DefaultCommitCoordinatorClientBuilder that DynamoDBCCClientBuilder doesn't need to extend:

CommitCoordinatorClient client = DynamoDBCCClientBuilder.buildCommitCoordinatorClient(
    "dynamodb", 
    config, 
    coordConf);

[prakharjain09]

@scottsand-db This class doesn't provide any method to iterate all configs. Does that work for UC Commit Coordinator? Don't we need to iterate over all configs in that? Will let @sumeet-db confirm.

[scottsand-db]

@prakharjain09 -- Yup, great callout. We can update APIs when we get there. IMO there's no rush or requirement to add it now.

[sumeet-db]

Will need a getAll / getAllByPrefix API for UC Commit Coordinator Builder when we get there.

[LukasRupprecht]

@LukasRupprecht and @sumeet-db -- is it every possible or intended/desired that AbstractMetadata and AbstractProtocol would be null? or are these required params? I see null being passed in in some existing tests today ...

They shouldn't be null as client implementations may rely on it (test code is an exception if we can be sure that the underlying client doesn't require it).