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

Fix failing file handler test #38

Merged
merged 1 commit into from
Jul 16, 2021
Merged

Fix failing file handler test #38

merged 1 commit into from
Jul 16, 2021

Conversation

sruti1312
Copy link
Contributor

@sruti1312 sruti1312 commented Jul 15, 2021

Signed-off-by: Sruti Parthiban partsrut@amazon.com

Is your feature request related to a problem? Please provide an existing Issue # , or describe.
#39
Fix failing event file handler test. Formatting changes was brought in by spotless.

Describe the solution you are proposing
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@sruti1312 sruti1312 requested a review from khushbr July 15, 2021 22:51
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2021

Codecov Report

Merging #38 (d77585a) into main (0ada819) will increase coverage by 0.15%.
The diff coverage is 57.89%.

❗ Current head d77585a differs from pull request most recent head 04236b3. Consider uploading reports for the commit 04236b3 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main      #38      +/-   ##
============================================
+ Coverage     72.09%   72.25%   +0.15%     
- Complexity      346      356      +10     
============================================
  Files            44       44              
  Lines          2473     2505      +32     
  Branches        155      160       +5     
============================================
+ Hits           1783     1810      +27     
- Misses          585      590       +5     
  Partials        105      105              
Impacted Files Coverage Δ
...performanceanalyzer/PerformanceAnalyzerPlugin.java 75.96% <ø> (-0.23%) ⬇️
...lectors/ShardIndexingPressureMetricsCollector.java 9.49% <0.00%> (ø)
...org/opensearch/performanceanalyzer/util/Utils.java 92.10% <ø> (ø)
...ormanceanalyzer/writer/EventLogQueueProcessor.java 61.44% <37.50%> (+3.11%) ⬆️
...analyzer/config/PerformanceAnalyzerController.java 79.20% <100.00%> (+1.23%) ⬆️
...config/PerformanceAnalyzerClusterConfigAction.java 87.23% <100.00%> (+0.87%) ⬆️
...nalyzer/collectors/ThreadPoolMetricsCollector.java 90.66% <0.00%> (+2.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c79039...04236b3. Read the comment docs.

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>
@jotok
Copy link
Contributor

jotok commented Jul 16, 2021

Is there an issue we can link to in the PR description?

Comment on lines +203 to +204
timeMillis % MetricsConfiguration.SAMPLING_INTERVAL
== 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems a little inefficient to test every number. What if we did something like this?

List<String> filesForCleanup = new ArrayList<>();

// ugly expression to handle the case when lastCleanupTimeBucket is divisible by SAMPLING_INTERVAL
// If I did this correctly, then k should satisfy the following: 
//   (k - 1) * SAMPLING_INTERVAL < lastCleanupTimeBucket <= k * SAMPLING_INTERVAL
long k = lastCleanupTimeBucket / SAMPLING_INTERVAL +
        lastCleanupTimebucket % SAMPLING_INTERVAL == 0 ? 0L : 1L;

for (int tick = k * SAMPLING_INTERVAL; tick < currCleanupTimeBucket; tick += SAMPLING_INTERVAL) {
  filesForCleanup.add(String::valueOf(tick));
}

Copy link
Member

@kkhatua kkhatua left a comment

Choose a reason for hiding this comment

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

LGTM.
Recommendation would have been to just fetch the list of "obsolete" files from the FS, but with a 1sec iteration, this might be an overhead. Probably the better approach is to test on the FS every 5 mins. But this isn't a blocker.

@sruti1312 sruti1312 merged commit 05314d2 into main Jul 16, 2021
@sruti1312 sruti1312 deleted the feature/bug-fix branch July 16, 2021 23:41
sruti1312 added a commit that referenced this pull request Aug 25, 2021
Signed-off-by: Sruti Parthiban <partsrut@amazon.com>
sruti1312 added a commit that referenced this pull request Aug 25, 2021
* Create writer file if metrics are available (#31)

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Add tests to check for writer file only if metrics are present (#35)

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Merge pull request #36 from opensearch-project/khushbr-writer-purge-fix

Fixing Event Log file cleanup issue

* Moving deleteAllFiles() to inside scheduleExecutor()

* Fixing the Link Checker errors, updating the official documentation

* nit: Fixing spotlessJava indentation issue

* Merge pull request #37 from khushbr/feature/purge-fix

Handling purging of lingering files before scheduleExecutor start.

* Fix failing file handler test (#38)

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Remove dependency on main branch when running spotless. (#47)

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Updates to gradle build file (#48)

* Updates to gradle build file

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Add ability to specify RCA branch

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

* Fix build when opensearch_version flag is provided. (#52)

Signed-off-by: Marc Handalian <handalm@amazon.com>

* Update the version to 1.0.1

Signed-off-by: Sruti Parthiban <partsrut@amazon.com>

Co-authored-by: Khushboo Rajput <59671881+khushbr@users.noreply.github.com>
Co-authored-by: Khushboo Rajput <khushbr@amazon.com>
Co-authored-by: Marc Handalian <handalm@amazon.com>
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