ibessonov commented on code in PR #1748:
URL: https://github.com/apache/ignite-3/pull/1748#discussion_r1132000857
##########
modules/core/src/main/java/org/apache/ignite/internal/causality/VersionedValue.java:
##########
@@ -568,22 +560,13 @@ private CompletableFuture<T>
previousOrDefaultValueFuture(long actualToken) {
/**
* Trims the storage to history size.
- *
- * @param causalityToken Last token which is being applied.
*/
- private void trimToSize(long causalityToken) {
- Long lastToken = history.lastKey();
+ private void trimToSize(NavigableMap<Long, CompletableFuture<T>> map) {
Review Comment:
Would it make sense to move synchronization inside of this method, just to
be explicit about its correctness?
##########
modules/core/src/main/java/org/apache/ignite/internal/causality/VersionedValue.java:
##########
@@ -187,34 +189,20 @@ private CompletableFuture<T> getInternal(long
causalityToken) {
long actualToken0 = this.actualToken;
if (history.floorEntry(causalityToken) == null) {
- throw new OutdatedTokenException(causalityToken, actualToken0,
historySize);
+ throw new OutdatedTokenException(causalityToken, actualToken0,
maxHistorySize);
}
Review Comment:
I have a weird feeling that VV will throw an exception here if it's not yet
initialized with any value, but is asked for the future. `history` map can be
empty, right?
If it can't, can you please explain why?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -588,6 +599,10 @@ private CompletableFuture<Void>
changeInternally0(ConfigurationSource src, long
throw new ConfigurationValidationException(validationIssues);
}
+ // "allChanges" map can be empty here in case the given update
matches the current state of the local configuration. We
+ // still try to write the empty update, because local
configuration can be obsolete. If this is the case, then the CAS will
+ // fail and the update will be recalculated and there is a chance
that the new local configuration will produce an non-empty
Review Comment:
I know my English sucks, but I think it's `a non-empty`, not `an non-empty`,
otherwise I simply don't get these rules. Can you please confirm whether you're
correct here or not?
##########
modules/core/src/test/java/org/apache/ignite/internal/causality/VersionedValueTest.java:
##########
@@ -208,14 +209,34 @@ public void testMissValueUpdate() throws
OutdatedTokenException {
*/
@Test
public void testObsoleteToken() {
- VersionedValue<Integer> longVersionedValue = new
VersionedValue<>(null);
+ VersionedValue<Integer> longVersionedValue = new
VersionedValue<>(null, 2, null);
Review Comment:
Please also rename it to `intVersionedValue` :)
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1931,74 +1938,116 @@ public void onError(Throwable e) {
private WatchListener createPendingAssignmentsRebalanceListener() {
return new WatchListener() {
@Override
- public void onUpdate(WatchEvent evt) {
+ public String id() {
+ return "pending-assignments-watch";
Review Comment:
Or this?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1857,17 +1857,22 @@ private RuntimeException convertThrowable(Throwable th)
{
private WatchListener createDistributionZonesDataNodesListener() {
return new WatchListener() {
@Override
- public void onUpdate(WatchEvent evt) {
+ public String id() {
+ return "dst-zones-data-nodes-watch";
Review Comment:
Should this be a constant?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1522,20 +1522,20 @@ private CompletableFuture<Void>
dropTableAsyncInternal(String name) {
dropTblFut.completeExceptionally(new
TableNotFoundException(DEFAULT_SCHEMA_NAME, name));
} else {
tablesCfg.change(chg ->
- chg.changeTables(tblChg -> {
- TableView tableCfg = tblChg.get(name);
+ chg.changeTables(tblChg -> {
+ TableView tableCfg = tblChg.get(name);
- if (tableCfg == null) {
- throw new
TableNotFoundException(DEFAULT_SCHEMA_NAME, name);
- }
+ if (tableCfg == null) {
+ throw new
TableNotFoundException(DEFAULT_SCHEMA_NAME, name);
+ }
- tblChg.delete(name);
- }).changeIndexes(idxChg -> {
- List<String> indicesNames =
tablesCfg.indexes().value().namedListKeys();
+ tblChg.delete(name);
+ }).changeIndexes(idxChg -> {
+ List<String> indicesNames =
tablesCfg.indexes().value().namedListKeys();
Review Comment:
Since you already changed this code, can I ask you to use `chg` instead of
`tablesCfg` and maybe extract `chg.indexes()` into a variable if necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
