Iceberg sink uses Parameters::get to ensure parameter definition exists

[liuml07]

Context

#170 (comment)

It looks like we should be using Parameters::get without the default value in this case. That would surface the runtime exception and hence the problem in this case.

Checklist

• ./gradlew build compiles code correctly
• Added new tests where applicable
• ./gradlew test passes all tests
• Extended README or added javadocs where applicable
• Added copyright headers for new files from CONTRIBUTING.md

[codyrioux]

Left a comment on the get method. I don't think it actually requires any action but worth a discussion. Otherwise LGTM!

[codyrioux]

Do the semantics actually change with this? My initial read of the code says no, but if it does it may unintentionally impact other jobs.

[liuml07]

No, all branches will assign value so it's redundant to have an initial value.

[liuml07]

But it's cosmetic and I can revert that. Or better I can rewrite the whole function to use multiple returns.

        try {
            final Object value = get(key);
            return value != null ? value : defaultValue;
        } catch (ParameterException ex) {
            return defaultValue;
        }

[sundargates]

What if you have a default value of null for the parameter and pass in a non-default value here as part of defaultValue? What should be the expected behavior? Can we also write tests for this to ensure the behavior remains consistent as we advance?

[liuml07]

That's a good question, but I really do not have the answer. The expected behavior depends on how it's now being used.

[liuml07]

According to this code snippet: https://github.com/Netflix/mantis/blob/master/mantis-runtime/src/main/java/io/mantisrx/runtime/parameter/ParameterUtils.java#L309-L315

It seems "default value of null for the parameter" means the parameter does not have a default value, i.e. missing in the definition. In that case, this get() method is doing the right thing: return the provided defaultValue. We can add a test for this.

[liuml07]

Actually some existing tests fail due to the test parameter "state" is not complete - not including all parameter definitions. So we will need to update the test code a bit. I'll make this ready for review after I fix that failing test in a non-intrusive way.

[github-actions]

Unit Test Results

109 files  +1  109 suites  +1   5m 34s ⏱️ +2s
477 tests +2  458 ✔️ +2  19 💤 ±0  0 ❌ ±0 

Results for commit d1d0c25. ± Comparison against base commit e6778e8.

♻️ This comment has been updated with latest results.

[liuml07]

failing test seems unrelated.

[liuml07]

All green. I'll merge by EoD. CC: @sundargates

[sundargates]

isn't junit-jupiter meant for usage with junit5?

[liuml07]

Yes. It works just fine if we only use the utility assertion methods from Jupiter. But I agree, it's better not to mix JUnit 4 and Jupiter. I have written the assertThrows in the testing code instead of pulling junitJupiter. I'll file a follow-up PR to upgrade JUnit 4 to Jupiter inmantis-runtime module.

[liuml07]

testJobComplete (io.mantisrx.master.jobcluster.JobClusterTest) failed but it seems unrelated. Let me merge from master and see if it's still failing.