Skip to content

Conversation

@moleske
Copy link
Member

@moleske moleske commented Jan 4, 2019

  • demonstrates commit and rollback of transactions
  • handles exceptions

 - demonstrates commit and rollback of transactions
 - handles exceptions

Co-authored-by: Michael Oleske <moleske@pivotal.io>
Co-authored-by: Blake Bender <bbender@pivotal.io>
Co-authored-by: Sai Boorlagaddda <sai@pivotal.io>
@moleske moleske requested review from igodwin and mmartell January 4, 2019 01:00
Copy link
Contributor

@jake-at-work jake-at-work left a comment

Choose a reason for hiding this comment

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

I’m approving with reservations. This example is very contrived and stinks of a unit test. Perhaps a better example shows an example that does some work, has an error (perhaps introduced randomly) and rolls back when the error occurs. We want to show examples of best practice implementation so doing work and then explicitly calling rollback doesn’t show an best practice, if you know you’re going to rollback best practice would be don’t do the work. ;)

@moleske
Copy link
Member Author

moleske commented Jan 4, 2019

I personally would be ok with those changes being requested, let's see if others have your reservations. Seems like it wouldn't be too crazy to throw some error and then rollback in something slightly more realistic.

@jake-at-work
Copy link
Contributor

Something like:

main() {
  ...
  transactionManager.begin();
  try {
    for (auto& key : batch) {
      auto value = getValueFromExternalSystem(key);
      cache.put(key, value);
    }
    transactionManager.commit();
  } catch ( ... ) {
    transactionManager.rollback();
  }
}

int32_t getValuFromExternalSystem(int32_t key) {
  if (random(10) == 0) {
    throw "failed to get from external system";
  }

  return random(key);
}

@pdxcodemonkey
Copy link
Contributor

I was getting stuck on the idea that we needed to force an exception external to the example code to have a "real" example. Charlie has suggested we implement a retry path as well, which also seems like a good suggestion. Thoughts?

Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

README correction, Line 1: Change 'Transaction Manger' to 'TransactionManager'.

- Make transaction more "real life" than just manually rollback and check
- Incorporate retry logic

Co-authored-by: Michael Oleske <moleske@pivotal.io>
@pdxcodemonkey
Copy link
Contributor

Le sigh - is there a modern C++ version that's not way more complex? STL stuff seems like overkill

- Use C++ 11 random number generation
- Move TransactionManager::begin into try/catch - NC sometimes throws IllegalStateException when calling this in a retry

Co-authored-by: Michael Oleske <moleske@pivotal.io>


int getValueFromExternalSystem() {
std::random_device rd{};
Copy link
Contributor

Choose a reason for hiding this comment

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

This compiles? So we have non-static rd passed to a static e. At best here we allocating more rd on each call that we will never use.

Copy link
Contributor

Choose a reason for hiding this comment

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

static thread_local std::default_random_engine generator(std::random_device{}());
auto value = std::uniform_int_distribution<int32_t>{0, 9}(generator);
...

Co-authored-by: Michael Oleske <moleske@pivotal.io>
@pdxcodemonkey pdxcodemonkey merged commit dd86276 into apache:develop Jan 4, 2019
@pdxcodemonkey pdxcodemonkey deleted the GEODE-6210-cpp-example-exceptions branch January 4, 2019 21:46
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.

4 participants