search

azkaban / azkaban-plugins

Plugins for Azkaban.
https://azkaban.github.io
Apache License 2.0
130 stars 178 forks source link

fix teradata jar missing issue #293

Closed kunkun-tang closed 6 years ago

kunkun-tang commented 6 years ago

Since we separated Teradata classes with other jobtype classes in https://github.com/azkaban/azkaban/pull/1717, it resulted in two individual jars: jobtype jar and Teradata Jar. Today Azkaban adds original jobtype dependency classpath by specifying the exact class name, retrieving which jar includes this class, and appending the jar into Job JVM starting command. For example, https://github.com/azkaban/azkaban/blob/7cd51237ad9e9f1994931cbe64f6dc1a640c0ece/az-hadoop-jobtype-plugin/src/main/java/azkaban/jobtype/HadoopJavaJob.java#L132

Because we separated the teradata and jobtype jar, we need to add teradata jar to classpath explicitly.

HappyRay commented 6 years ago

What classpath? It's not obvious to me what the problem was and how the change fixed it. Yes, I have looked at the diff. Could you provide more details?

kunkun-tang commented 6 years ago

Explained more in the Description Panel. @HappyRay

HappyRay commented 6 years ago

Do other plugins work in the same way in terms of classpath?

HappyRay commented 6 years ago

Could you think of automated tests to catch this regression?

HappyRay commented 6 years ago

Per our earlier agreement, every time we find a bug, by default we need to add automated tests to prevent a similar issue in the future.

kunkun-tang commented 6 years ago

I just checked other plugins, like PinotBuildAndPush. Its classpath is included inside the JVM starting commands, like /export/apps/azkaban/azkaban-exec-server/azkaban-exec-server_10688/plugins/jobtypes/PinotBuildAndPushJob/lib/*

So, in the future refactors, we may place teradata jars into every individual teradata related plugins.

HappyRay commented 6 years ago

So, in the future refactors, we may place teradata jars into every individual teradata related plugins.

Merge state

Which approach do you think is better?

kunkun-tang commented 6 years ago

I agree that we need to invest on the automated test. However, this code will eventually be discarded in later future. The jobtype automated test should be redeisgned with the new jobtype proposal. As we see, manual calling class name in java code was not a reliable design choice, and we should replace it with new approach.

In this PR, we might not want to introduce the change. Alternatively, we may

kunkun-tang commented 6 years ago

Which approach do you think is better?

Like Reportal dependency, we'd place teradata abstract dependency under every teradata-replated plugin/lib.

HappyRay commented 6 years ago

Like Reportal dependency, we'd place teradata abstract dependency under every teradata-replated plugin/lib.

When do you plan to do it?

kunkun-tang commented 6 years ago

When do you plan to do it?

The bottleneck is that Teradata codebase relies on maual-check-in jars. We need to figure it out firstly.

kunkun-tang commented 6 years ago

I misunderstood your question. Yes, I believe we should be able to do it at the current stage.

HappyRay commented 6 years ago

What's your plan then?

kunkun-tang commented 6 years ago

Instead of root/lib path, we should add the build generated jar, az-teradata-jdbc.jar, into every plugin/lib folder. We need to make sure plugin's classpath includes it.

HappyRay commented 6 years ago

When do you plan to implement what you stated above? Is that tracked?