Fix Topic Operator's reconciliationIntervalMs type

[fvaleri]

The reconciliationIntervalMs should be of type long, as it is dealing with milliseconds, and it is also defined as long in the User Operator model class.

[scholzj]

Why should it be long?

[fvaleri]

I started from the model, where we have the reconciliationTimeMs and long is more appropriate for this time resolution. During a test, I set a value of ms in reconciliationIntervalSeconds by mistake and the operator failed with a not so great message (no indication of the offending configuration):

Exception in thread "main" io.strimzi.operator.common.InvalidConfigurationException: Failed to parse. Negative value is not supported for this configuration

If you prefer, we can use int for the spec, but then I would do the same for the User Operator, and possibly improve the configuration error message in a different PR.

[ppatierno]

I am fine to align them because the meaning of the variable is the same. So long for both should work.

[scholzj]

To be honest, I do not think this is a good enough reason to change it from int to long. int seems fully sufficient for this field. The field is also very clear that it is in seconds. I think it should be kept as it is (it would be great if we could change the UO field to int as well, but that might break compatibility, so I guess we have to leave that as long).

[fvaleri]

I do not think this is a good enough reason to change it from int to long

When you use the env var STRIMZI_FULL_RECONCILIATION_INTERVAL_MS you have ms, then you remember that there is a spec option for that, so you just copy and paste that value, but no, that's in seconds. This is how I did that mistake.

Don't you think that consistency is important here? Why the TO and UO should behave differently? See also my proposal in the next comment.

it would be great if we could change the UO field to int as well, but that might break compatibility, so I guess we have to leave that as long

Yeah, that's not an option now. We may get this change and log an improvement for v1, where we can break compatibility. In that case, we could move both to int, or keep long to align with the env var, maybe renaming to reconciliationTimeMs. I'm fine with both.

[scholzj]

When you use the env var STRIMZI_FULL_RECONCILIATION_INTERVAL_MS you have ms, then you remember that there is a spec option for that, so you just copy and paste that value, but no, that's in seconds. This is how I did that mistake.

Is it confusing to use milliseconds in one place (although slightly hidden from the user) and seconds in another one? Likely yes, but we cannot change the API easily. Feel free to open an issue and we can try to improve it in the v1 API if you think this is important enough.

But changing an int to long as you do here does not fix this problem. And it is probably better if you get an error here than if it happily accepts your millisecond value as seconds value.

Don't you think that consistency is important here? Why the TO and UO should behave differently? See also my proposal in the next comment.

It is important to use consistently the correct things. Not be consistent in doing the wrong thing.

Also, as a sidenote ... there is not much consistency between the TO and the other operators. In the PR next to this one, you seem to be happy to continue and extend the inconsistencies there (for exaample by using the var fields instead of using proper types). Yet here you are somehow pretending that consistency is the most important thing you cannot live without?

[fvaleri]

It is important to use consistently the correct things. Not be consistent in doing the wrong thing.

Fair enough. I'll remove the TO spec change from this PR and log an issue.

consistency is the most important thing you cannot live without?

Never said that. I was talking about behavior consistency, which is a different thing. If you think there is some issue with using var, please comment on the specific context and I'll be happy to discuss and make improvements.

[scholzj]

Never said that. I was talking about behavior consistency, which is a different thing. If you think there is some issue with using var, please comment on the specific context and I'll be happy to discuss and make improvements.

I do not see any inconsistent behavior here in this case. There is inconsistent use of int and long, but no inconsistent behavior.

As for var, I think this was already discussed at least once in the past. It is only very rarely used in our code outside of the Topic Operator. As I said, I do not have a major problem with it as I accepted that the TO is special and different for one reason or another. I'm just pointing out that you seem happy with inconsistencies in one place and very strict about them in another.

[fvaleri]

There is inconsistent use of int and long, but no inconsistent behavior.

If you insert a value that overflows int, then you have a different behavior. The UO will take it, while the TO will fail (which is better, as you say).

[fvaleri]

@scholzj spec changes are reverted now.

[scholzj]

The code LGTM. Please update the PR description.

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ppatierno]

LGTM

[ppatierno]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).