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 counts and add_new_item methods, without tests #4

Merged
merged 47 commits into from
Oct 27, 2023

Conversation

Oxygen588
Copy link
Contributor

@Oxygen588 Oxygen588 commented Jul 3, 2023

Close #3 , Close #2 ;
Modified the addItem method to not allow duplicate jobs to be added.
Created a new method that gets the length atomically.

Oxygen588 and others added 10 commits June 30, 2023 11:16
Implemented no duplicate items based on id (A update would be creating the id based on the data hash, as well as currently within some languages add item method getting the list of item could be done using pipeline)
Implemented tests for each one of the languages for the no duplicate items.
Fixed some issues related to run bash file.
Within Job spawner modified the way shared jobs work so its not within the main list as well as fixed the way the host and port is being set.
Python test fixed the way the host and port is being set.
AddAtomicItem method was fixed by using transactions instead of pipeline.
The normal AddItem was added back.
GetQueueLengthsAtomic function as well as the AddAtomicItem function has been created for the atomic way of adding items, and the normal AddItem function has been added back.
Change in name for the atomic tests function call
Moved the start of job spawner before all the tests so there is no conflict between the new tests.
Old Add_Item function has been added back, the atomic add_item is now add_atomic_item and has been fixed to run atomically as well as the get_queue_lengths in now atomic fixed as well.
The add_atomic_item has been fixed now, the add_item function has been added back, The tests for duplicate items has been fixed.
Small update to tests, forgot to uncomment the add of the second item.
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
Oxygen588 added 5 commits July 4, 2023 13:33
Removed non Atomic way of getting queue length and renamed the GetQueueLengthsAtomic to GetQueueLengths
Watch atomicity implemented within go-lang.
AddAtomicItem Fix!
Added warning message.
The atomic function fully works and tests have been modified to support locking!
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
@JOT85
Copy link
Member

JOT85 commented Jul 4, 2023

Sorry I didn't have your latest commit when writing that last review. I'll update my comments.

go/WorkQueue.go Outdated Show resolved Hide resolved
Oxygen588 added 6 commits July 4, 2023 22:31
TS addAtomicItem fix using watch.
'AddAtomicItem Rename' to 'AddItemAtomically'.
Re-written the comment for AddItemAtomically.
Better Watch for loop.
AddItemAtomically: removed unnecessary pipe.
Improved the return of the lengths function.
Removed the limit of tries until it stops trying.
The new fix implements the redis watch. This should make it fully atomic.
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
Oxygen588 added 2 commits July 5, 2023 13:59
… fixed return at Lengths function Go

Unexecuted pipe fix.
addAtomicItem Pipes fixed and executed properly as well as necessary checks for transaction errors or generic errors has been fixed
Fixed unexecuted pipe and properly checked the pipe for items already in list.
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
tests/rust/src/main.rs Outdated Show resolved Hide resolved
tests/rust/src/main.rs Outdated Show resolved Hide resolved
tests/run-test.sh Outdated Show resolved Hide resolved
tests/rust/src/main.rs Outdated Show resolved Hide resolved
@@ -42,6 +42,7 @@ while [[ $# -gt 0 ]]; do
esac
done

python3 job-spawner-and-cleaner.py "$host" "$tests" &
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member

Choose a reason for hiding this comment

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

How come this moved from line 108 to line 45?

rust/src/lib.rs Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Show resolved Hide resolved
Python: Fixed Add_new_item, fixed exception handling.
Rust: Made `get_queue_lengths` not async, now returns future.
Test: There was no need for `python3 job-spawner-and-cleaner.py "$host" "$tests"` it was already running at line 45.
tests/rust/Cargo.toml Outdated Show resolved Hide resolved
tests/rust/src/main.rs Outdated Show resolved Hide resolved
tests/rust/src/main.rs Outdated Show resolved Hide resolved
@@ -42,6 +42,7 @@ while [[ $# -gt 0 ]]; do
esac
done

python3 job-spawner-and-cleaner.py "$host" "$tests" &
Copy link
Member

Choose a reason for hiding this comment

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

How come this moved from line 108 to line 45?

tests/job-spawner-and-cleaner.py Outdated Show resolved Hide resolved
tests/go/main.go Outdated Show resolved Hide resolved
Re-written the way tests are done for `addNewItem` now 3 functions that spawn 10 `addNewItem` calls for the item we are trying to add and runned concurrently.

TODO: Currently because of all added logic the difference in speed in between go and node & python, the difference in speed is much higher because of that the difference in number of shared-jobs done is much higher.
go/WorkQueue.go Outdated Show resolved Hide resolved
go/WorkQueue.go Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
node/src/WorkQueue.ts Outdated Show resolved Hide resolved
python/redis_work_queue/workqueue.py Outdated Show resolved Hide resolved
rust/src/lib.rs Outdated Show resolved Hide resolved
Oxygen588 and others added 5 commits September 29, 2023 10:10
Moved `addNewItem` with sleep for tests, from the lib to the tests.
…atting

According to the docs at https://pkg.go.dev/github.com/redis/go-redis/v9#Client.Watch, we don't need
to `Exec` the pipeline.

Furthermore, the redis library ensures the database is safe to use multiple transactions
concurrently, so I've removed that from the doc comment.
go/WorkQueue.go Outdated
Comment on lines 149 to 162
workQueue.AddItemToPipeline(ctx, pipe, item)
return nil
})
return err
} else {
return nil
}
}

for {
err := db.Watch(ctx, txf, workQueue.processingKey, workQueue.mainQueueKey)
if err == nil {
return true, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

This returns true even if the item isn't added!!


const transaction = db.multi();
this.addItemToPipeline(transaction, item);
const results = await transaction.exec();
Copy link
Member

Choose a reason for hiding this comment

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

@Oxygen588 what does the return value look like in the success and failure case? I'm not sure what it would be, so it would be nice to have a comment!

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to clarify what I mean, I don't know what the value of results if the transaction succeeds or fails.

go/WorkQueue.go Outdated
@@ -151,7 +151,7 @@ func (workQueue *WorkQueue) AddNewItem(
})
return err
} else {
return nil
return false
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what's going on here? 😂

@JOT85 JOT85 changed the title Atomic length method & no duplicate Jobs Add counts and add_new_item methods, without tests Oct 27, 2023
@JOT85 JOT85 changed the base branch from main to atomic-methods October 27, 2023 12:40
Copy link
Member

@JOT85 JOT85 left a comment

Choose a reason for hiding this comment

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

Still needs tests, but we'll merge it into atomic-methods and add tests on that branch.

@JOT85 JOT85 merged commit 11abffe into MeVitae:atomic-methods Oct 27, 2023
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.

Atomic length method Don't add duplicate jobs
2 participants