Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

Change the atomicity mode of some caches to transactional to fix issue 682 #683

Closed
wants to merge 45 commits into from

Conversation

pkommoju
Copy link
Contributor

@pkommoju pkommoju commented Aug 27, 2021

Change all caches in Port Manager to transactional atomicity. Mixing caches of different atomicity mode under a transaction does not work in Ignite. This was causing a failure in concurrent deletion of ports.

Linked to Issue #682

pkommoju and others added 30 commits May 12, 2021 11:34
Add document about transactional semantics for Alcor Caches in general
and spefically about Ignite Caches.
@pkommoju pkommoju requested a review from cj-chung August 27, 2021 00:32
@xieus xieus added the bug Something isn't working label Aug 27, 2021
@xieus xieus added this to the Version 0.18.2021.08.30 milestone Aug 27, 2021
@xieus xieus linked an issue Aug 27, 2021 that may be closed by this pull request
Copy link
Contributor

@xieus xieus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkommoju A few comments.

Comment on lines +1 to +4
<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation=" http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">
<bean id="grid.cfg" class="org.apache.ignite.configuration.IgniteConfiguration">
<property name="peerClassLoadingEnabled" value="true" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this file for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was for reference only but it can go away now.

this.cache = igniteClient.getOrCreateCache(clientCacheConfig);
logger.log(Level.INFO, "Retrieved cache " + this.cache.getConfiguration().getName() + " AtomicityMode is " + this.cache.getConfiguration().getAtomicityMode());
} catch (ClientException e) {
logger.log(Level.WARNING, "Create cache for client " + cacheConfig.getName() + " failed:" + e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty bad exception. Need to raise the logger level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change it to ERROR.

this.cache = ignite.getOrCreateCache(cfg);
logger.log(Level.WARNING, "Create cache for client " + cfg + " failed:" + e.getMessage());
} catch (Exception e) {
logger.log(Level.WARNING, "Unexpected failure:" + e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERROR.

NeighborInfo.class, getNeighborCacheName(entry.getKey()));
CacheConfiguration cfg = new CacheConfiguration();
cfg.setName(getNeighborCacheName(entry.getKey()));
cfg.setAtomicityMode(CacheAtomicityMode.TRANSACTIONAL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not hard code the atomicity mode here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a dynamic cache so it had to be created with hardcoded atomicity mode set to whatever we want but since some other static caches have TRANSACTIONAL mode, this one is hardcoded.

Better alternative is to have the service config set the atomicity mode and in the code, create the cache in that mode. There is not going to be any use case for atomistic caches, so we need to think about alternative mechanisms.

@lgtm-com
Copy link

lgtm-com bot commented Aug 27, 2021

This pull request introduces 1 alert and fixes 1 when merging 5dcec47 into b4a4288 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

fixed alerts:

  • 1 for Boxed variable is never null

@xieus
Copy link
Contributor

xieus commented Sep 10, 2021

How about this PR? @pkommoju We are seeing quite a few of conflicting files.

@pkommoju
Copy link
Contributor Author

Yes, this is expected because changes in two branches are getting combined into one. Let me take a look at them and attach a patch/comments about what to keep what to remove.

@pkommoju
Copy link
Contributor Author

Abandon this PR and branch. I didn't realize it quickly enough.

PR#685 already brought in all these changes.

@xieus
Copy link
Contributor

xieus commented Sep 10, 2021

Close this PR as it is duplicated with PR #685.

@xieus xieus closed this Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PM] PM Failed to delete port when concurrent
3 participants