Deprecate reflection-based JobTest apply method

[rubanm]

This can cause some dependency related pains. And the associated jobs will always be a compile-time dependency I suppose.

[benpence]

+1

[johnynek]

out of curiosity, why this change? In fact, Scalding Tool launches reflectively, so this at least exercises the same path.

Also, so many old jobs use this, that this is just adding to the deprecation warnings, no?

[benpence]

@johnynek If the Scalding Tool launches reflectively, we probably shouldn't depend on the JobTest constructor to catch problems with it right? Or do you mean as a means of ensuring the user's specific job class takes Args as a constructor etc?

@rubanm what are the dependency problems?

[johnynek]

the JobTest reflective version is exactly the same code path that Tool uses, so it is testing that the reflective creation works.

With any function, it is conceivable that it would not work with Tool.

Execution, of course, does not have this issue.

[jnievelt]

The main reason we don't like this apply is that it doesn't depend on an import. If we were to use it, probably we'd anyway do a Class#getName, which is exactly what the Manifest variant does.

I would almost have suggested we deprecate the Manifest construction as well but, as you say, it's testing a legitimate Tool requirement that the class has a Args constructor.

I think the only thing we enable with the String construction is being able to build a test without depending on the Job class at compile time. Of course you'd need to include it somehow at runtime, so it seems unlikely to provide a real benefit in testing.

[rubanm]

There is a new pants goal being rolled out internally to automatically warn/error about unused dependencies. Some unit test packages that use the reflection based apply were running into false positives.

It's a general issue with reflection, rather than anything specific in Job or JobTest. I can discard this change if keeping it helps with better test coverage.

[johnynek]

well, I think reflection sucks, don't get me wrong, so I'm happy to nuke it, I just was afraid it was going to add to ignored warning noise at Twitter.

[rubanm]

@johnynek Cool. I forgot to mention that @stuhood already replaced all usages with the non reflection based ones. So hopefully we don't add much noise with this change.

[johnynek]

While we are at it, can we use ClassTag rather than Manifest which was
deprecated?

On Tue, Jul 5, 2016 at 8:05 AM, Ruban Monu notifications@github.com wrote:

@johnynek https://github.com/johnynek Cool. I forgot to mention that
@stuhood https://github.com/stuhood already replaced all usages with
the non reflection based ones. So hopefully we don't add much noise with
this change.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1578 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAEJdni9CHNa3gKV2vMoLPFEYgNC2Qcnks5qSpz1gaJpZM4JCswg
.

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin