Confusing/wrong topology version in GridCacheVersion

[Denis Magda]

Igniters,

While working on IGNITE-946
(https://issues.apache.org/jira/browse/IGNITE-946) I faced with a
strange part of the code.

GridCacheVersion class contains a getter that returns topology version.
But this is not a plain cluster's topology version we know that is
increased by a coordinator each time the topology changes.
It's, according to GridCacheVersion documentation, /"topology version
*plus number of seconds* from the start time of the first grid node".
/The documentation is not wrong, just have a look at
GridCacheVersionManager.next() implementation.

IMHO, this looks like a mess.

What is the reason to have such so called "topologyVersion"?

If there is a reason for that why this sum is called "topologyVersion"?
It's not just confusing but rather will lead to the bugs in the code.
Moreover, this is seems to be a bug caused by improper naming:

if (addTime) {
     if (gridStartTime ==0)
         gridStartTime =cctx.kernalContext().discovery().gridStartTime();

     topVer += (gridStartTime -TOP_VER_BASE_TIME) /1000;
}

long globalTime =cctx.kernalContext().clockSync().adjustedTime(topVer);


In the code above topVer+time is passed to adjustedTime() method, but
plain topVer should have been as I guess (GridCacheVersionManager.next()).

--
Denis

[Alexey Goncharuk]

Denis,

Since I think this is a valuable part of the ticket design, I copied this
email to the ticket comments and responded there, please take a look.

2015-08-10 3:19 GMT-07:00 Denis Magda <[hidden email]>:

> Igniters,
>
> While working on IGNITE-946 (
> https://issues.apache.org/jira/browse/IGNITE-946) I faced with a strange
> part of the code.
>
> GridCacheVersion class contains a getter that returns topology version.
> But this is not a plain cluster's topology version we know that is
> increased by a coordinator each time the topology changes.
> It's, according to GridCacheVersion documentation, /"topology version
> *plus number of seconds* from the start time of the first grid node".
> /The documentation is not wrong, just have a look at
> GridCacheVersionManager.next() implementation.
>
> IMHO, this looks like a mess.
>
> What is the reason to have such so called "topologyVersion"?
>
> If there is a reason for that why this sum is called "topologyVersion"?
> It's not just confusing but rather will lead to the bugs in the code.
> Moreover, this is seems to be a bug caused by improper naming:
>
> if (addTime) {
>     if (gridStartTime ==0)
>         gridStartTime =cctx.kernalContext().discovery().gridStartTime();
>
>     topVer += (gridStartTime -TOP_VER_BASE_TIME) /1000;
> }
>
> long globalTime =cctx.kernalContext().clockSync().adjustedTime(topVer);
>
>
> In the code above topVer+time is passed to adjustedTime() method, but
> plain topVer should have been as I guess (GridCacheVersionManager.next()).
>
> --
> Denis
>