Skip to content

SNAP-1950#361

Open
suranjan wants to merge 12 commits intosnappy/masterfrom
SNAP-1950-2
Open

SNAP-1950#361
suranjan wants to merge 12 commits intosnappy/masterfrom
SNAP-1950-2

Conversation

@suranjan
Copy link

@suranjan suranjan commented Feb 1, 2018

This is to avoid a race where GII first takes pendingTXRegionState lock
and then tries to apply the operation which in turn tries to create
the TXRegionState.
In case of BatchMessage, we first create the TXRegionState and then try
to take lock on pendingTXRegionState. This causes cyclic dependency and
deadlock

Changes proposed in this pull request

(Fill in the changes here)

Patch testing

(Fill in the details about how this patch was tested)

ReleaseNotes changes

(Does this change require an entry in ReleaseNotes? If yes, has it been added to it?)

Other PRs

(Does this change require changes in other projects- snappydata, spark, spark-jobserver, aqp? Add the links of PR of the other subprojects that are related to this change)

Suranjan Kumar added 3 commits January 31, 2018 21:18
  This is to avoid a race where GII first takes pendingTXRegionState lock
  and then tries to apply the operation which in turn tries to create
  the TXRegionState.
  In case of BatchMessage, we first create the TXRegionState and then try
  to take lock on pendingTXRegionState. This causes cyclic dependency and
  deadlock
@suranjan suranjan requested review from kneeraj and sumwale February 1, 2018 06:40
Suranjan Kumar added 2 commits February 1, 2018 12:34
  Instead of NonReentrantReadWriteLock, use StoppableReentrantReadWriteLock
@suranjan suranjan changed the title Take pendingTxRegionStates lock in TXBatchMessage SNAP-1950 Feb 1, 2018
Suranjan Kumar added 5 commits February 1, 2018 17:54
  In case of co-ordinator wait for initialization
  In the TXRegionState constructor check if write lock
  already taken In that case don't try to take lock again.
Copy link

@sumwale sumwale left a comment

Choose a reason for hiding this comment

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

See few comments.

if (!r.isInitialized() && r.getImageState().lockPendingTXRegionStates(true, false)) {
lockedRegions.add(r);
}
}*/
Copy link

Choose a reason for hiding this comment

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

commented portion can be removed to avoid confusion

region.getImageState().lockPendingTXRegionStates(true, false)) {
//lockedRegions.add(region);
lockedPendingTXregionState = true;
}
Copy link

Choose a reason for hiding this comment

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

cleaner to do "boolean lockPendingTXRegionState = !region.isInitialized() && region.getImageState().lock..."

boolean alreadyLocked = imgState.isWriteLockedBySameThread();
if (!r.isInitialized()
&& (alreadyLocked || imgState.lockPendingTXRegionStates(true, false))) {
try {
Copy link

Choose a reason for hiding this comment

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

Since the lock can be taken multiple times by same thread, won't it be better to make this a ReentrantReadWriteLock instead of "isWriteLockedBySameThread" check? The block inside will add TXRegionState in pending list and initialize pendingTXOps lists which I think is required.

@sumwale sumwale force-pushed the snappy/master branch 2 times, most recently from 337768c to ec453a6 Compare April 8, 2022 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants