Add custom JAVA_TOOL_OPTIONS by SdkRunnableTask

[andresgomezfrr]

Read then delete this section

Allow configuring custom JAVA_TOOL_OPTIONS by SdkRunnableTask

Tests

I tested it manually and it worked fine:

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Issue

flyteorg/flyte#3910

[honnix]

Minor comments, otherwise LGTM.

[honnix]

Please create an issue and reference to it for visibility, as this is a pretty useful new feature.

[honnix]

Hmm, this is actually problematic. flytekit-java is shipped from user code space, which means for existing code, getCustomJavaToolOptions does not exist in SdkRunnableTask. So here for now we need to use reflection to check existence of this method, until we migrate all users.

[andresgomezfrr]

You're right... this is the case where a user uses an old version of flytekit-java and tries to register with a new version of it, right?

[honnix]

Let's me think about it. It may not be a problem. I need to check how jflyte works.

[honnix]

Forgot about my previous comment. I somehow thought this code was in jflyte module.

I think we will need to have default implementation in RunnableTask after all. jflyte will use flytekit-java module from user code space when registering because of ChildFirstClassLoader, so when processing old version where RunnableTaskImpl does not have getCustomJavaToolOptions, ProjectClosure will fail when calling task.getCustomJavaToolOptions(), or maybe even earlier because old RunnableTaskImpl does not really implement new RunnableTask if there is no default implementation.

It might come as a common rule that there must be default implementation in RunnableTask whenever we extend it, so the existing FIXMEs are not telling the truth.

[andresgomezfrr]

so.. in this case, should I remove all the FIXME comments?

[andresgomezfrr]

I tested it using the old flytekit-java in the user repo and registered the task with the new flytekit-java container and with the default property, everything worked fine.

[honnix]

so.. in this case, should I remove all the FIXME comments?

I think it makes sense. It is rather simple to keep the defaults than asking users to migrate, and the migration itself doesn't bring much value anyway.

[honnix]

I tested it using the old flytekit-java in the user repo and registered the task with the new flytekit-java container and with the default property, everything worked fine.

Great. Sorry I gave wrong direction in an early comment. I did not think carefully about those "FIXME"s .

[andresgomezfrr]

Don't worry! so.. looks good to you the PR now?

[honnix]

This breaks backward compatibility. Please check my comment https://github.com/flyteorg/flytekit-java/pull/238/files#r1279358563.

[honnix]

There are some test failures.

[andresgomezfrr]

There are some test failures.

yes... I just saw this.. checking

[andresgomezfrr]

There are some test failures.

I think that this test return a duplicated 😮

[ERROR] Failures: 
[ERROR]   SdkNodeNamePolicyTest.nextNodeIdShouldReturnUniquePrefixForDistinctPolicies:71 returned duplicated prefix
Expected: a collection with size <9>
     but: collection size was <10>

  @Test
  void nextNodeIdShouldReturnUniquePrefixForDistinctPolicies() {
    var prefixes =
        Stream.generate(SdkNodeNamePolicy::new)
            .limit(10)
            .map(SdkNodeNamePolicy::nextNodeId)
            .map(SdkNodeNamePolicyTest::prefix)
            .collect(toList());
    var uniquePrefixes = Set.copyOf(prefixes);

    assertThat("returned duplicated prefix", prefixes, hasSize(uniquePrefixes.size()));
  }