Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core/198/handle stale transactions #359

Merged
merged 6 commits into from
Sep 7, 2016

Conversation

rhsimplex
Copy link
Contributor

@rhsimplex rhsimplex commented Jun 6, 2016

addresses #198

@codecov-io
Copy link

codecov-io commented Jun 6, 2016

Current coverage is 93.46% (diff: 95.83%)

Merging #359 into master will increase coverage by 0.98%

@@             master       #359   diff @@
==========================================
  Files            22         23     +1   
  Lines          1209       1254    +45   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1118       1172    +54   
+ Misses           91         82     -9   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 90cc84a...84ccd01

@rhsimplex rhsimplex force-pushed the core/198/handle-stale-transactions branch from ef64c94 to 1da8950 Compare July 20, 2016 13:26
@rhsimplex rhsimplex force-pushed the core/198/handle-stale-transactions branch 3 times, most recently from cf939ee to 6a774da Compare August 12, 2016 08:31
@@ -70,6 +70,7 @@ def start(self):
p_map_bigchain.start()
logger.info('starting block')
block.start()
stale.start()
Copy link
Contributor

Choose a reason for hiding this comment

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

add a logger.info

@rhsimplex
Copy link
Contributor Author

Ok i don't know how to fix the test coverage. The only two untested lines left are the configuration. I've tried adding backlog_reassign_delay in test_commands.py and test_config_utils.py in the last two commits but apparently that's not helping. Suggestion?

@@ -136,11 +138,54 @@ def write_transaction(self, signed_transaction, durability='soft'):
signed_transaction = deepcopy(signed_transaction)
# update the transaction
signed_transaction.update({'assignee': assignee})
signed_transaction.update({'assignment_timestamp': time()})
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using time.time() maybe we should use bigchaindb.util.timestamp()
It should not make any difference in this case but I feel it would be more consistent and this would not break if in the future we change how federation nodes get their time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think it will work -- for stale transactions, we need to look within a window of time. A string time stamp may not order correctly (e.g. "100" comes before "20"). For instance, you can't call RethinkDBs between function on "0" and "140000000" and get the right answer, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

timestamp returns a fixed length string. You can always convert to int before writing it to the database. I am just concerned of problems that may arise if we start getting timestamps from different sources in different parts of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then this shouldn't be too difficult

@r-marques
Copy link
Contributor

r-marques commented Sep 7, 2016

Another thing that is missing is that the block creation pipeline now needs to also listen for updates.

# The first commit's message is:
stale transaction monitor and tests

# The 2nd commit message will be skipped:

#	simplify logic

# The 3rd commit message will be skipped:

#	node will assign to self

# The 4th commit message will be skipped:

#	block listens for insert and update

# The 5th commit message will be skipped:

#	more test coverage

# The 6th commit message will be skipped:

#	test coverage

# The 7th commit message will be skipped:

#	test coverage
@r-marques
Copy link
Contributor

r-marques commented Sep 7, 2016

cc @vrde
This is failing right now because the UPDATE from the changefeed returns {"old_val": ..., "new_val": ...} instead of the transaction

Two solutions would be:

  • change the ChangeFeed class to return only new_val
  • in block.filter_tx check if its a transaction or update notification and do if update tx = update['new_val']

@rhsimplex rhsimplex force-pushed the core/198/handle-stale-transactions branch from 51ce43f to 84ccd01 Compare September 7, 2016 14:17
@r-marques
Copy link
Contributor

works on my computer so I think we are good to go 👍

@vrde
Copy link
Contributor

vrde commented Sep 7, 2016

works on my computer so I think we are good to go

😂

@rhsimplex rhsimplex merged commit 92981e0 into master Sep 7, 2016
@rhsimplex rhsimplex deleted the core/198/handle-stale-transactions branch September 7, 2016 14:26
sbellem pushed a commit to sbellem/bigchaindb that referenced this pull request Oct 24, 2016
* add timestamp to transaction assignment

* add reassignment delay to configuration

* refactor to multipipes

* # This is a combination of 7 commits.
# The first commit's message is:
stale transaction monitor and tests

# The 2nd commit message will be skipped:

#	simplify logic

# The 3rd commit message will be skipped:

#	node will assign to self

# The 4th commit message will be skipped:

#	block listens for insert and update

# The 5th commit message will be skipped:

#	more test coverage

# The 6th commit message will be skipped:

#	test coverage

# The 7th commit message will be skipped:

#	test coverage

* stale transaction monitor and tests

* update operation only returns new value
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.

None yet

5 participants