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

Add support for writing String data into bigqueue #49

Merged
merged 3 commits into from
Mar 24, 2019
Merged

Conversation

mangalaman93
Copy link
Member

@mangalaman93 mangalaman93 commented Mar 20, 2019

Fixes #19

benchmarks

$ go test -bench=BenchmarkStringDoubleCopy
goos: linux
goarch: amd64
pkg: github.com/grandecola/bigqueue
BenchmarkStringDoubleCopy-8   	10000000	       151 ns/op
PASS
ok  	github.com/grandecola/bigqueue	8.464s
$ go test -bench=BenchmarkStringNoCopy
goos: linux
goarch: amd64
pkg: github.com/grandecola/bigqueue
BenchmarkStringNoCopy-8   	10000000	       112 ns/op
PASS
ok  	github.com/grandecola/bigqueue	7.8788

@mangalaman93 mangalaman93 added the enhancement New feature or request label Mar 20, 2019
@mangalaman93 mangalaman93 added this to the v0.3.0 milestone Mar 20, 2019
@mangalaman93 mangalaman93 self-assigned this Mar 20, 2019
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #49 into master will decrease coverage by 1.2%.
The diff coverage is 83.87%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #49      +/-   ##
=========================================
- Coverage      82%   80.8%   -1.21%     
=========================================
  Files           7       7              
  Lines         289     323      +34     
=========================================
+ Hits          237     261      +24     
- Misses         27      32       +5     
- Partials       25      30       +5
Impacted Files Coverage Δ
arena.go 70.37% <100%> (ø) ⬆️
config.go 100% <100%> (ø) ⬆️
write.go 84% <100%> (+9.64%) ⬆️
read.go 76% <75.92%> (-0.93%) ⬇️
bigqueue.go 90% <81.81%> (-10%) ⬇️
arenamanager.go 71.42% <0%> (-2.39%) ⬇️

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 99aa1cf...9f199f3. Read the comment docs.

@mangalaman93 mangalaman93 force-pushed the aman/string branch 2 times, most recently from c3b864b to e0991ac Compare March 23, 2019 14:14
@mangalaman93 mangalaman93 marked this pull request as ready for review March 23, 2019 14:15
@mangalaman93 mangalaman93 force-pushed the aman/string branch 3 times, most recently from e532bfe to 1a14376 Compare March 23, 2019 18:33
@mangalaman93
Copy link
Member Author

@ashish-goswami this is ready to merge now.

@@ -45,7 +48,7 @@ func NewBigQueue(dir string, opts ...Option) (*BigQueue, error) {
}
defer func() {
if !complete {
index.close()
_ = index.close()
Copy link
Member

Choose a reason for hiding this comment

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

why are we not checking for errors here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even when we check for errors, the only thing we could do is to log the error. I didn't want to have dependency on log library in bigqueue.

@mangalaman93 mangalaman93 merged commit 930fcde into master Mar 24, 2019
@mangalaman93 mangalaman93 deleted the aman/string branch March 24, 2019 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants