Skip to content

WIP: MINIFICPP-1318 move test rocksdb state to /var/tmp#863

Closed
szaszm wants to merge 2 commits intoapache:mainfrom
szaszm:MINIFICPP-1318
Closed

WIP: MINIFICPP-1318 move test rocksdb state to /var/tmp#863
szaszm wants to merge 2 commits intoapache:mainfrom
szaszm:MINIFICPP-1318

Conversation

@szaszm
Copy link
Copy Markdown
Member

@szaszm szaszm commented Aug 5, 2020

Missed TailFileTests state storage in #862


Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file?
  • If applicable, have you updated the NOTICE file?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

Copy link
Copy Markdown
Contributor

@fgerlits fgerlits left a comment

Choose a reason for hiding this comment

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

There are a lot more:

~/src/minifi$ git grep '"/tmp' | wc -l
147

the rest do not need to be changed?

@szaszm
Copy link
Copy Markdown
Member Author

szaszm commented Aug 5, 2020

There are a lot more:

~/src/minifi$ git grep '"/tmp' | wc -l
147

the rest do not need to be changed?

They are fine as long as we don't try to create a rocksdb database there. The problem is that rocksdb is configured to use direct IO, i.e. bypass the page cache, but tmpfs is just a mounted page cache without backing storage, so it makes no sense to use direct IO there.

@arpadboda
Copy link
Copy Markdown
Contributor

@szaszm : not sure if the CI failures we face on U16 jobs are related or not, but I would prefer to investigate before proceeding with this.

@szaszm szaszm changed the title MINIFICPP-1318 move test rocksdb state to /var/tmp WIP: MINIFICPP-1318 move test rocksdb state to /var/tmp Aug 9, 2020
@fgerlits
Copy link
Copy Markdown
Contributor

@szaszm : not sure if the CI failures we face on U16 jobs are related or not, but I would prefer to investigate before proceeding with this.

I am 99% sure that both of those failures in TailFileTests are due to the bug fixed in #859. That PR has been merged now, so rebasing on top of it should get rid of the test failures.

commit d5d1654
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Thu Aug 6 11:25:25 2020 +0200

    MINIFICPP-1312 - Remove unnecessary duration_cast

commit 841cd73
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Thu Aug 6 09:59:51 2020 +0200

    MINIFICPP-1312 - Review changes

commit c4394dc
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Tue Aug 4 15:10:13 2020 +0200

    MINIFICPP-1312 - Review changes

commit 267d5a4
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Tue Aug 4 13:56:46 2020 +0200

    MINIFICPP-1312 - Remove some more logs

commit 85e191d
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Tue Aug 4 13:55:07 2020 +0200

    MINIFICPP-1312 - Remove extra log messages

commit 9584fff
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Mon Aug 3 15:56:15 2020 +0200

    MINIFICPP-1312 - Busy wait for condition

commit 76406ef
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Mon Aug 3 11:33:47 2020 +0200

    MINIFICPP-1312 - More logs

commit ba23458
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Mon Aug 3 10:21:02 2020 +0200

    MINIFICPP-1312 - More logs

commit 68c6efd
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Mon Aug 3 09:50:42 2020 +0200

    MINIFICPP-1312 - Linter fix

commit 114fa50
Author: Adam Debreceni <adam.debreceni@protonmail.com>
Date:   Mon Aug 3 09:19:58 2020 +0200

    MINIFICPP-1312 - Log generously

Signed-off-by: Marton Szasz <szaszm01@gmail.com>
@szaszm szaszm marked this pull request as draft August 24, 2020 11:29
auto root = testController.root_;

int timeout_ms = 1000;
unsigned int timeout_ms = 1000;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As this is a duration its type could also be std::chrono::duration, with no _ms suffix needed then.

@szaszm
Copy link
Copy Markdown
Member Author

szaszm commented Sep 1, 2020

The CI issue is fixed in #879, thanks to @fgerlits

@szaszm szaszm closed this Sep 1, 2020
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.

5 participants