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

Remove unused materializer code and completed threshold #2382

Merged
merged 3 commits into from Sep 15, 2020

Conversation

erimatnor
Copy link
Contributor

@erimatnor erimatnor commented Sep 14, 2020

  • The new refresh functionality for continuous aggregates replaces the
    old materializer, which means some code is no longer used and should
    be removed.
  • The completed threshold in the TimescaleDB catalog is no longer used
    by the refactored continuous aggregates, so it is removed.
  • This change cleans up and removes duplicate code for internal lookups
    of continuous aggregates. A number of related error messages have also
    been cleaned up and made conformant with the error style guide.

Fixes #2178
Closes #2395

@erimatnor erimatnor added this to the 2.0.0 milestone Sep 14, 2020
@erimatnor erimatnor changed the title Caggs cleanup old code Remove unused materializer code and completed threshold Sep 14, 2020
erimatnor added a commit to erimatnor/timescaledb that referenced this pull request Sep 14, 2020
This change filters materialized hypertables from the hypertables
view, similar to how internal compression hypertables are
filtered.

Materialized hypertables are internal objects created as a side effect
of creating a continuous aggregate, and these internal hypertables are
still listed in the continuous_aggregates view.

Fixes timescale#2382
@erimatnor erimatnor force-pushed the caggs-cleanup-old-code branch 2 times, most recently from ab124e2 to 7c72a39 Compare September 14, 2020 08:43
erimatnor added a commit to erimatnor/timescaledb that referenced this pull request Sep 14, 2020
This change filters materialized hypertables from the hypertables
view, similar to how internal compression hypertables are
filtered.

Materialized hypertables are internal objects created as a side effect
of creating a continuous aggregate, and these internal hypertables are
still listed in the continuous_aggregates view.

Fixes timescale#2382
erimatnor added a commit to erimatnor/timescaledb that referenced this pull request Sep 14, 2020
This change filters materialized hypertables from the hypertables
view, similar to how internal compression hypertables are
filtered.

Materialized hypertables are internal objects created as a side effect
of creating a continuous aggregate, and these internal hypertables are
still listed in the continuous_aggregates view.

Fixes timescale#2382
@erimatnor erimatnor force-pushed the caggs-cleanup-old-code branch 2 times, most recently from 9ba5afa to 1daffc5 Compare September 14, 2020 22:26
@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #2382 into master will increase coverage by 1.16%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2382      +/-   ##
==========================================
+ Coverage   88.91%   90.08%   +1.16%     
==========================================
  Files         212      213       +1     
  Lines       34788    34299     -489     
==========================================
- Hits        30932    30898      -34     
+ Misses       3856     3401     -455     
Impacted Files Coverage Δ
src/catalog.c 84.70% <ø> (ø)
src/catalog.h 100.00% <ø> (ø)
src/cross_module_fn.c 58.13% <ø> (+1.32%) ⬆️
src/hypertable.c 87.42% <ø> (-0.08%) ⬇️
tsl/src/continuous_aggs/insert.c 87.23% <ø> (+0.15%) ⬆️
tsl/src/continuous_aggs/materialize.c 66.00% <ø> (+52.70%) ⬆️
tsl/src/init.c 88.88% <ø> (ø)
src/process_utility.c 93.59% <88.88%> (-0.05%) ⬇️
src/chunk.c 95.31% <100.00%> (-0.01%) ⬇️
src/continuous_agg.c 93.50% <100.00%> (+4.18%) ⬆️
... and 8 more

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 01fc0de...6251a39. Read the comment docs.

Copy link
Contributor

@pmwkaa pmwkaa left a comment

Choose a reason for hiding this comment

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

Looks good, looked through and did not find any issues

Copy link
Contributor

@k-rus k-rus left a comment

Choose a reason for hiding this comment

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

LGTM
Can you add to commit that it is also part of #2395?

I don't understand why you cannot reuse rel_name and need to obtain again in few places to report in errors. See my comments in the code for exact place.

src/chunk.c Outdated Show resolved Hide resolved
src/chunk.c Outdated Show resolved Hide resolved
src/chunk.c Outdated Show resolved Hide resolved
src/continuous_agg.c Outdated Show resolved Hide resolved
src/continuous_agg.c Show resolved Hide resolved
src/process_utility.c Show resolved Hide resolved
src/continuous_agg.c Show resolved Hide resolved
@erimatnor erimatnor force-pushed the caggs-cleanup-old-code branch 4 times, most recently from 6b6a425 to 2bf4ac3 Compare September 15, 2020 14:49
This change cleans up and removes duplicate code for internal lookups
of continuous aggregates. A number of related error messages have also
been cleaned up and made conformant with the error style guide.
The new refresh functionality for continuous aggregates replaces the
old materializer, which means some code is no longer used and should
be removed.

Closes timescale#2395
The completed threshold in the TimescaleDB catalog is no longer used
by the refactored continuous aggregates, so it is removed.

Fixes timescale#2178
@erimatnor erimatnor merged commit 5179447 into timescale:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants